From 03697190ffe886bdbe1352b23d352c06ef0b2e7d Mon Sep 17 00:00:00 2001 From: Jakub Syty Date: Wed, 9 Sep 2026 16:44:32 +0200 Subject: [PATCH] Handle exit code properly for warnings in git diff output (#3426) --- packages/web/server/lib/git/DOCUMENTATION.md | 1 + packages/web/server/lib/git/service.js | 48 ++++++-------- packages/web/server/lib/git/service.test.js | 66 ++++++++++++++++++++ 3 files changed, 86 insertions(+), 29 deletions(-) diff --git a/packages/web/server/lib/git/DOCUMENTATION.md b/packages/web/server/lib/git/DOCUMENTATION.md index 1017c1bb..5964297d 100644 --- a/packages/web/server/lib/git/DOCUMENTATION.md +++ b/packages/web/server/lib/git/DOCUMENTATION.md @@ -137,6 +137,7 @@ The following functions are internal helpers used by exported functions: - Commit comparison uses the same server boundary through optional `GitAPI.getGitCommitDiff`. Desktop Changes, mobile Changes, and the existing walkthrough surface share branch/commit comparison semantics. Mobile Changes uses the same selectors and `useGitComparison` file-list owner, with a read-only list-to-detail flow. VS Code keeps its existing modes because its Git bridge does not provide these comparison operations. The HTTP operations are available to web, Electron, hosted mobile, and Capacitor clients. ### Staged and unstaged change handling +- Untracked patches from `getDiff` and `getUntrackedDiffs` use `git diff --no-index` with separate stdout, stderr, and process exit status. Exit codes 0 and 1 return stdout only, so line-ending warnings never become patch text or request failures. Other exits and process failures reject the single-file request; the batch keeps an empty entry for the failed path and preserves the other results. - `status.files` exposes both `index` and `working_dir` codes. Shared UI uses these as separate scopes: staged rows are derived from non-empty `index` statuses, while unstaged rows are derived from `working_dir` statuses and untracked files. - A file with both staged and unstaged changes can appear in both UI sections. Staged rows request diffs with `staged: true`; unstaged rows request normal working-tree diffs. - The shared Git panel exposes explicit staging actions. Unstaged rows use `stageFile`, staged rows use `unstageFile`, and commits operate on the current staged index. diff --git a/packages/web/server/lib/git/service.js b/packages/web/server/lib/git/service.js index 28a0b760..04082902 100644 --- a/packages/web/server/lib/git/service.js +++ b/packages/web/server/lib/git/service.js @@ -942,7 +942,7 @@ const runGitCommand = async (cwd, args) => { } catch (error) { return { success: false, - exitCode: typeof error?.code === 'number' ? error.code : 1, + exitCode: Number.isInteger(error?.code) ? error.code : null, stdout: String(error?.stdout || ''), stderr: String(error?.stderr || ''), message: parseGitErrorText(error), @@ -2465,6 +2465,21 @@ export async function getStatus(directory, options = {}) { } } +const getNoIndexDiff = async (repoRoot, repoPath, contextLines) => { + const args = ['diff', '--no-color']; + if (Number.isFinite(contextLines)) { + args.push(`-U${Math.max(0, contextLines)}`); + } + args.push('--no-index', '--', '/dev/null', repoPath); + const result = await runGitCommand(repoRoot, args); + // Exit 1 means differences, even when Git also writes warnings to stderr. + // Spawn and buffer errors have no numeric exit code and must still fail. + if (result.exitCode === 0 || result.exitCode === 1) { + return result.stdout; + } + throw new Error(result.stderr || result.message || 'Failed to get untracked Git diff'); +}; + export async function getDiff(directory, { path: filePath, staged = false, contextLines = 3 } = {}) { const { directoryPath, directoryGit, repoRoot, git } = await createRepositoryGitContext(directory); @@ -2515,21 +2530,7 @@ export async function getDiff(directory, { path: filePath, staged = false, conte ].join('\n'); } - const noIndexArgs = ['diff', '--no-color']; - if (typeof contextLines === 'number' && !Number.isNaN(contextLines)) { - noIndexArgs.push(`-U${Math.max(0, contextLines)}`); - } - noIndexArgs.push('--no-index', '--', '/dev/null', fileContext.repoPath); - try { - const noIndexDiff = await git.raw(noIndexArgs); - return noIndexDiff; - } catch (noIndexError) { - // git diff --no-index returns exit code 1 when differences exist (not a real error) - if (noIndexError.exitCode === 1 && noIndexError.message) { - return noIndexError.message; - } - throw noIndexError; - } + return await getNoIndexDiff(repoRoot, fileContext.repoPath, contextLines); } } catch (error) { console.error('Failed to get Git diff:', error); @@ -2578,7 +2579,7 @@ export async function getUntrackedDiffs(directory, filePaths = [], { concurrency const paths = (Array.isArray(filePaths) ? filePaths : []).filter((value) => typeof value === 'string' && value); if (paths.length === 0) return []; - const { directoryPath, directoryGit, repoRoot, git } = await createRepositoryGitContext(directory); + const { directoryPath, directoryGit, repoRoot } = await createRepositoryGitContext(directory); const results = new Array(paths.length).fill(''); let cursor = 0; @@ -2587,18 +2588,7 @@ export async function getUntrackedDiffs(directory, filePaths = [], { concurrency const index = cursor++; try { const fileContext = await resolveGitFileContext(directoryPath, directoryGit, paths[index], repoRoot); - const args = ['diff', '--no-color']; - if (typeof contextLines === 'number' && !Number.isNaN(contextLines)) { - args.push(`-U${Math.max(0, contextLines)}`); - } - args.push('--no-index', '--', '/dev/null', fileContext.repoPath); - try { - results[index] = await git.raw(args); - } catch (error) { - // `git diff --no-index` exits 1 whenever there are differences, which - // for a new file is always. - results[index] = error?.exitCode === 1 && error?.message ? error.message : ''; - } + results[index] = await getNoIndexDiff(repoRoot, fileContext.repoPath, contextLines); } catch { results[index] = ''; } diff --git a/packages/web/server/lib/git/service.test.js b/packages/web/server/lib/git/service.test.js index a641e1ee..23b34da2 100644 --- a/packages/web/server/lib/git/service.test.js +++ b/packages/web/server/lib/git/service.test.js @@ -35,6 +35,7 @@ import { unstageFiles, applyHunk, getDiff, + getUntrackedDiffs, getFileDiff, validateWorktreeCreate, parseBranchCreationSource, @@ -363,6 +364,71 @@ describe('applyHunk', () => { }); }); +describe.runIf(canRunGit())('untracked diffs', () => { + it.each(['false', 'warn'])('returns only the patch with core.safecrlf=%s', async (safecrlf) => { + const { tmpDir, git } = await createTempRepo(); + await git.addConfig('core.autocrlf', 'true'); + await git.addConfig('core.safecrlf', safecrlf); + fs.writeFileSync(path.join(tmpDir, 'new file.txt'), 'first\nsecond\n'); + + // Confirm this fixture produces a real diff exit, including stderr in the warning case. + let expectedPatch; + try { + runGit(tmpDir, ['diff', '--no-color', '--no-index', '--', '/dev/null', 'new file.txt']); + throw new Error('Expected git diff to exit with differences'); + } catch (error) { + expect(error.status).toBe(1); + expectedPatch = error.stdout; + if (safecrlf === 'warn') { + expect(error.stderr).toContain('LF will be replaced by CRLF'); + } + } + + const diff = await getDiff(tmpDir, { path: 'new file.txt' }); + expect(diff).toBe(expectedPatch); + expect(diff).toContain('+first\n+second\n'); + expect(diff).not.toContain('warning:'); + expect(await getUntrackedDiffs(tmpDir, ['new file.txt'])).toEqual([diff]); + }); + + it('accepts an empty untracked file without a process error', async () => { + const { tmpDir } = await createTempRepo(); + fs.writeFileSync(path.join(tmpDir, 'empty.txt'), ''); + const diff = await getDiff(tmpDir, { path: 'empty.txt' }); + expect(diff).toContain('new file mode 100644'); + expect(diff).not.toContain('@@'); + expect(await getUntrackedDiffs(tmpDir, ['empty.txt'])).toEqual([diff]); + }); + + it('rejects fatal conversion errors while preserving other batch entries', async () => { + const { tmpDir } = await createTempRepo(); + runGit(tmpDir, ['config', 'diff.broken.textconv', 'false']); + fs.writeFileSync(path.join(tmpDir, '.gitattributes'), 'bad.txt diff=broken\n'); + fs.writeFileSync(path.join(tmpDir, 'first.safe'), 'first\n'); + fs.writeFileSync(path.join(tmpDir, 'bad.txt'), 'bad\n'); + fs.writeFileSync(path.join(tmpDir, 'last.safe'), 'last\n'); + + await expect(getDiff(tmpDir, { path: 'bad.txt' })).rejects.toThrow('unable to read files to diff'); + const diffs = await getUntrackedDiffs(tmpDir, ['first.safe', 'bad.txt', 'last.safe'], { concurrency: 1 }); + expect(diffs).toHaveLength(3); + expect(diffs[0]).toContain('+first\n'); + expect(diffs[1]).toBe(''); + expect(diffs[2]).toContain('+last\n'); + }); + + it('rejects truncated patches when the process output exceeds the buffer limit', async () => { + const { tmpDir } = await createTempRepo(); + fs.writeFileSync(path.join(tmpDir, 'large.txt'), 'x'.repeat(21 * 1024 * 1024) + '\n'); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + await expect(getDiff(tmpDir, { path: 'large.txt' })).rejects.toThrow('maxBuffer'); + expect(await getUntrackedDiffs(tmpDir, ['large.txt'])).toEqual(['']); + } finally { + errorSpy.mockRestore(); + } + }); +}); + describe('symlink diffs', () => { it('treats an untracked directory symlink as a link in patch and split diffs', async () => { if (!canRunGit() || process.platform === 'win32') return;