From 8bf7b1c8142d58ad3d3b0ede9b0b087b17e334b3 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sat, 22 Aug 2026 20:13:21 -0700 Subject: [PATCH] fix: respect Git line-ending normalization in working-tree diffs The worktree diff passed --ignore-cr-at-eol to the whole Git command whenever the new endpoint was the working tree. That is a global override: it suppressed carriage-return differences even in files whose .gitattributes marks them as binary or -text, where CRLF is meaningful. Drop the flag and let Git's own attribute handling decide per path. A file marked `text eol=crlf` is already normalized on read, so a one-line edit in a CRLF checkout still diffs as one line, which is the case this PR set out to fix. A file marked `-text` keeps its carriage returns visible, which is the case the review raised. The regression test now covers both in one repository: a normalized .ts file whose single-line edit stays a single-line diff, and an HTTP fixture marked -text whose CRs appear in the patch. --- .changeset/git-line-ending-normalization.md | 5 ++ .../default/vcs/git/commands.test.ts | 46 +++++++++++++++ src/extensions/default/vcs/git/commands.ts | 6 +- src/extensions/default/vcs/git/index.test.ts | 58 +++++++++++++++++++ src/extensions/default/vcs/git/index.ts | 21 ++++--- 5 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 .changeset/git-line-ending-normalization.md diff --git a/.changeset/git-line-ending-normalization.md b/.changeset/git-line-ending-normalization.md new file mode 100644 index 000000000..1e5ab2b9f --- /dev/null +++ b/.changeset/git-line-ending-normalization.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Respect Git line-ending normalization when reviewing working-tree changes. diff --git a/src/extensions/default/vcs/git/commands.test.ts b/src/extensions/default/vcs/git/commands.test.ts index 8cc906be7..0579ac7f8 100644 --- a/src/extensions/default/vcs/git/commands.test.ts +++ b/src/extensions/default/vcs/git/commands.test.ts @@ -17,6 +17,7 @@ import { resolveGitMetadata, runGitText, shouldSkipLargeTrackedDiff, + type GitDiffEndpoints, } from "./commands"; import type { ExtensionVcsDiffInput as VcsDiffCommandInput } from "hunkdiff/extension"; @@ -182,6 +183,51 @@ describe("git command helpers", () => { expect(buildGitDiffArgs(makeGitInput({ range: "main..feature" }))).toContain("main..feature"); }); + test("never ignores CR at EOL globally", () => { + const worktreeEndpoints = { + old: { kind: "index" }, + new: { kind: "worktree" }, + } satisfies GitDiffEndpoints; + const singleRefWorktreeEndpoints = { + old: { kind: "git-ref", ref: "old-ref" }, + new: { kind: "worktree" }, + } satisfies GitDiffEndpoints; + const storedComparisons = [ + { + input: makeGitInput({ staged: true }), + endpoints: { + old: { kind: "git-ref", ref: "old-ref" }, + new: { kind: "index" }, + }, + }, + { + input: makeGitInput({ range: "old-ref..new-ref" }), + endpoints: { + old: { kind: "git-ref", ref: "old-ref" }, + new: { kind: "git-ref", ref: "new-ref" }, + }, + }, + ] satisfies { input: VcsDiffCommandInput; endpoints: GitDiffEndpoints }[]; + + expect(buildGitDiffArgs(makeGitInput(), [], null, worktreeEndpoints)).not.toContain( + "--ignore-cr-at-eol", + ); + expect( + buildGitDiffArgs(makeGitInput({ range: "old-ref" }), [], null, singleRefWorktreeEndpoints), + ).not.toContain("--ignore-cr-at-eol"); + expect(buildGitDiffNumstatArgs(makeGitInput(), worktreeEndpoints)).not.toContain( + "--ignore-cr-at-eol", + ); + expect( + buildGitDiffNumstatArgs(makeGitInput({ range: "old-ref" }), singleRefWorktreeEndpoints), + ).not.toContain("--ignore-cr-at-eol"); + + for (const { input, endpoints } of storedComparisons) { + expect(buildGitDiffArgs(input, [], null, endpoints)).not.toContain("--ignore-cr-at-eol"); + expect(buildGitDiffNumstatArgs(input, endpoints)).not.toContain("--ignore-cr-at-eol"); + } + }); + test("disables external diff tools for stash patches", () => { const args = buildGitStashShowArgs({ kind: "stash-show", diff --git a/src/extensions/default/vcs/git/commands.ts b/src/extensions/default/vcs/git/commands.ts index 161554a8e..93574d891 100644 --- a/src/extensions/default/vcs/git/commands.ts +++ b/src/extensions/default/vcs/git/commands.ts @@ -159,6 +159,7 @@ export function buildGitDiffArgs( input: ExtensionVcsDiffInput, excludedPathspecs: string[] = [], colorMoved: GitColorMovedOptions | null = null, + _endpoints: GitDiffEndpoints | null = null, ) { const args = ["diff", "--no-ext-diff", "--find-renames", ...gitPatchColorArgs(colorMoved)]; @@ -185,7 +186,10 @@ export function buildGitDiffArgs( } /** Build the cheap tracked-file stats query used to skip huge file diffs before patch output. */ -export function buildGitDiffNumstatArgs(input: ExtensionVcsDiffInput) { +export function buildGitDiffNumstatArgs( + input: ExtensionVcsDiffInput, + _endpoints: GitDiffEndpoints | null = null, +) { const args = ["diff", "--no-ext-diff", "--find-renames", "--no-color", "--numstat", "-z"]; if (input.staged) { diff --git a/src/extensions/default/vcs/git/index.test.ts b/src/extensions/default/vcs/git/index.test.ts index fc43fe21c..ad21ffb13 100644 --- a/src/extensions/default/vcs/git/index.test.ts +++ b/src/extensions/default/vcs/git/index.test.ts @@ -43,6 +43,11 @@ function normalizeComparablePath(path: string) { return resolvedPath.replace(/\\/g, "/"); } +/** Remove Git's optional moved-line colors before asserting patch text semantics. */ +function stripAnsi(text: string) { + return text.replace(/\x1b\[[0-?]*[ -/]*[@-~]/g, ""); +} + function git(cwd: string, ...cmd: string[]) { const proc = Bun.spawnSync(["git", ...cmd], { cwd, @@ -171,6 +176,59 @@ describe("GitVcsAdapter", () => { expect(await result.readFileSource?.({ ...file, side: "new" })).toBe("new\ncontext\n"); }); + test("preserves per-path line-ending attributes in worktree diffs", async () => { + const repo = createTempRepo("hunk-git-adapter-crlf-"); + git(repo, "config", "core.autocrlf", "false"); + mkdirSync(join(repo, "fixtures")); + writeFileSync(join(repo, ".gitattributes"), "*.ts text eol=crlf\nfixtures/*.http -text\n"); + writeFileSync( + join(repo, "fixtures", "request.http"), + "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n", + ); + const originalLines = Array.from( + { length: 10_001 }, + (_, index) => `export const value${index} = ${index};`, + ); + writeFileSync(join(repo, "example.ts"), `${originalLines.join("\n")}\n`); + git(repo, "add", ".gitattributes", "example.ts", "fixtures/request.http"); + git(repo, "commit", "-m", "initial"); + + // Make Git materialize the committed attributes, then preserve CRLF while editing one line. + writeFileSync(join(repo, "example.ts"), "dirty\n"); + git(repo, "checkout", "--", "example.ts"); + + const input = { + kind: "vcs", + staged: false, + options: {}, + } satisfies ExtensionVcsDiffInput; + const operation = GitVcsAdapter.operations["working-tree-diff"]!; + expect(operation.watchSignature!(input, { cwd: repo })).toBe(""); + + const changedLines = [...originalLines]; + changedLines[5_000] = "export const value5000 = 50_000;"; + writeFileSync(join(repo, "example.ts"), `${changedLines.join("\r\n")}\r\n`); + writeFileSync(join(repo, "fixtures", "request.http"), "GET / HTTP/1.1\nHost: example.com\n\n"); + + const result = await operation.load(input, { cwd: repo }); + const plainPatch = stripAnsi(result.patchText); + const patchLines = plainPatch.split("\n"); + const stats = { + additions: patchLines.filter((line) => line.startsWith("+") && !line.startsWith("+++")) + .length, + deletions: patchLines.filter((line) => line.startsWith("-") && !line.startsWith("---")) + .length, + }; + + expect(stats).toEqual({ additions: 4, deletions: 4 }); + expect(result.extraFiles ?? []).toHaveLength(0); + expect(plainPatch).toContain("-export const value5000 = 5000;"); + expect(plainPatch).toContain("+export const value5000 = 50_000;"); + expect(plainPatch).toContain("diff --git a/fixtures/request.http b/fixtures/request.http"); + expect(plainPatch).toContain("-GET / HTTP/1.1\r"); + expect(operation.watchSignature!(input, { cwd: repo })).toBe(plainPatch); + }); + test("loads revision and stash patches through adapter operations", async () => { const repo = createTempRepo("hunk-git-adapter-show-"); writeFileSync(join(repo, "file.txt"), "one\n"); diff --git a/src/extensions/default/vcs/git/index.ts b/src/extensions/default/vcs/git/index.ts index d3a1b27ff..a80c116f7 100644 --- a/src/extensions/default/vcs/git/index.ts +++ b/src/extensions/default/vcs/git/index.ts @@ -179,10 +179,9 @@ function createGitRevisionSourceCapability( function createGitDiffSourceCapability( input: ExtensionVcsDiffInput, repoRoot: string, - cwd: string, + endpoints: GitDiffEndpoints | null, gitExecutable: string, ): GitSourceCapability | undefined { - const endpoints = resolveGitDiffEndpoints(input, { cwd, repoRoot, gitExecutable }); return endpoints ? createGitSourceCapability(input, repoRoot, endpoints, gitExecutable) : undefined; @@ -283,6 +282,7 @@ export function createGitVcsAdapter({ "working-tree-diff": { async load(input, { cwd }) { const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable }); + const endpoints = resolveGitDiffEndpoints(input, { cwd, repoRoot, gitExecutable }); const repoName = basename(repoRoot); const range = describeDiffRange(input); const title = input.staged @@ -293,13 +293,18 @@ export function createGitVcsAdapter({ // Ask for stats before the patch so files too large to render can be // excluded from the diff instead of generating output nobody reads. const largeTrackedFiles = parseGitNumstat( - runGitText({ input, args: buildGitDiffNumstatArgs(input), cwd, gitExecutable }), + runGitText({ + input, + args: buildGitDiffNumstatArgs(input), + cwd, + gitExecutable, + }), ).filter((file) => shouldSkipLargeTrackedDiff(file, repoRoot)); const colorMoved = resolveGitColorMovedOptions(input, { cwd, gitExecutable }); const sourceCapability = createGitDiffSourceCapability( input, repoRoot, - cwd, + endpoints, gitExecutable, ); @@ -338,14 +343,14 @@ export function createGitVcsAdapter({ return buildGitWatchPlan(input, cwd, gitExecutable); }, watchSignature(input, { cwd }) { - const trackedPatch = runGitText({ - input, - args: buildGitDiffArgs(input), + const repoRoot = resolveGitRepoRoot(input, { cwd, gitExecutable, preventOptionalLocks: true, }); - const repoRoot = resolveGitRepoRoot(input, { + const trackedPatch = runGitText({ + input, + args: buildGitDiffArgs(input), cwd, gitExecutable, preventOptionalLocks: true,