From abf6c1329120b326cf443919dba06847f195d611 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 3 Oct 2026 21:12:42 +0900 Subject: [PATCH] fix: preserve line endings when updating a file Updates split the file on LF and joined it back with LF, so a one-line patch rewrote every line of a CRLF or mixed-ending file. The file is now parsed into lines that keep their own ending; a chunk is replaced around its context lines, which keep their exact text and ending; inserted lines take the file's first ending. This is Codex's PreserveLineEndings model (text_file.rs, compute_replacements). Fixes #47 --- CHANGELOG.md | 1 + src/index.ts | 31 ++++----- src/line-endings.ts | 90 ++++++++++++++++++++++++++ test/line-endings.test.ts | 130 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 234 insertions(+), 18 deletions(-) create mode 100644 src/line-endings.ts create mode 100644 test/line-endings.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 396483f..1debc8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Recognize indented file headers like Codex, and reject any other line between file sections instead of silently dropping that section while reporting success (#45). +- Preserve each line's ending on update: CRLF and mixed-ending files are no longer rewritten to LF, inserted lines take the file's line ending, and context lines keep their exact text (#47). ### Changed diff --git a/src/index.ts b/src/index.ts index 8d69839..3e98659 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,6 +13,7 @@ import { import { Box, Container, Spacer, Text } from "@earendil-works/pi-tui"; import * as Diff from "diff"; import { Type } from "typebox"; +import { type ContextLineIndex, type LineReplacement, replacementsAroundContext, SourceText } from "./line-endings.js"; import { writeFileAtomic } from "./write-file-atomic.js"; const APPLY_PATCH_PARAMS = Type.Object({ @@ -30,6 +31,7 @@ type PatchChunk = { changeContexts: string[]; oldLines: string[]; newLines: string[]; + contextLineIndices: ContextLineIndex[]; isEndOfFile: boolean; }; @@ -1030,6 +1032,7 @@ function parsePatch(patchText: string): ParsedPatch[] { const oldLines: string[] = []; const newLines: string[] = []; + const contextLineIndices: ContextLineIndex[] = []; let isEndOfFile = false; let parsedLines = 0; while (index < endIndex) { @@ -1048,9 +1051,11 @@ function parsePatch(patchText: string): ParsedPatch[] { const prefix = hunkLine[0]; const value = hunkLine.slice(1); if (prefix === undefined) { + contextLineIndices.push([oldLines.length, newLines.length]); oldLines.push(""); newLines.push(""); } else if (prefix === " ") { + contextLineIndices.push([oldLines.length, newLines.length]); oldLines.push(value); newLines.push(value); } else if (prefix === "-") { @@ -1071,7 +1076,7 @@ function parsePatch(patchText: string): ParsedPatch[] { if (parsedLines === 0) { throw new PatchParseError("Update hunk does not contain any lines"); } - chunks.push({ changeContexts, oldLines, newLines, isEndOfFile }); + chunks.push({ changeContexts, oldLines, newLines, contextLineIndices, isEndOfFile }); } if (chunks.length === 0 && !movePath) { throw new PatchParseError(`Update file hunk for path '${filePath}' is empty`); @@ -1106,17 +1111,10 @@ function parseNonEmptyPatch(patchText: string): ParsedPatch[] { throw new PatchParseError("apply_patch verification failed: no hunks found"); } -function splitFileLines(content: string): string[] { - const lines = normalizePatchText(content).split("\n"); - if (lines[lines.length - 1] === "") { - lines.pop(); - } - return lines; -} - function replaceChunks(content: string, filePath: string, chunks: PatchChunk[]): { content: string; fuzz: number } { - const originalLines = splitFileLines(content); - const replacements: { start: number; oldLength: number; newLines: string[] }[] = []; + const source = SourceText.parse(content); + const originalLines = source.texts; + const replacements: LineReplacement[] = []; let lineIndex = 0; let fuzz = 0; @@ -1153,16 +1151,13 @@ function replaceChunks(content: string, filePath: string, chunks: PatchChunk[]): } fuzz += foundAt.fuzz; - replacements.push({ start: foundAt.index, oldLength: pattern.length, newLines }); + replacements.push( + ...replacementsAroundContext(foundAt.index, pattern.length, newLines, chunk.contextLineIndices), + ); lineIndex = foundAt.index + pattern.length; } - const nextLines = [...originalLines]; - for (const replacement of replacements.sort((left, right) => right.start - left.start)) { - nextLines.splice(replacement.start, replacement.oldLength, ...replacement.newLines); - } - nextLines.push(""); - return { content: nextLines.join("\n"), fuzz }; + return { content: source.replace(replacements), fuzz }; } async function applySingleHunk( diff --git a/src/line-endings.ts b/src/line-endings.ts new file mode 100644 index 0000000..545397a --- /dev/null +++ b/src/line-endings.ts @@ -0,0 +1,90 @@ +type LineEnding = "\n" | "\r\n" | "\r"; + +type SourceLine = { readonly text: string; readonly ending: LineEnding | undefined }; + +export type LineReplacement = { readonly start: number; readonly oldLength: number; readonly newLines: string[] }; + +/** `[index in oldLines, index in newLines]` of a context line, which sits on both sides of a chunk. */ +export type ContextLineIndex = readonly [number, number]; + +/** + * Splits a matched chunk into replacements that skip its context lines, so those source lines keep + * their exact text and ending (Codex `compute_replacements`, PreserveLineEndings mode). Context + * indices past the matched region belong to a trailing empty line the matcher already dropped. + */ +export function replacementsAroundContext( + start: number, + oldLength: number, + newLines: string[], + contextLineIndices: readonly ContextLineIndex[], +): LineReplacement[] { + const replacements: LineReplacement[] = []; + let oldStart = 0; + let newStart = 0; + for (const [oldContext, newContext] of contextLineIndices) { + if (oldContext >= oldLength || newContext >= newLines.length) break; + if (oldStart !== oldContext || newStart !== newContext) { + replacements.push({ + start: start + oldStart, + oldLength: oldContext - oldStart, + newLines: newLines.slice(newStart, newContext), + }); + } + oldStart = oldContext + 1; + newStart = newContext + 1; + } + if (oldStart !== oldLength || newStart !== newLines.length) { + replacements.push({ + start: start + oldStart, + oldLength: oldLength - oldStart, + newLines: newLines.slice(newStart), + }); + } + return replacements; +} + +/** + * A file split into lines that remember their own line ending, so an update rewrites only the lines a + * patch touches. Mirrors Codex `codex-rs/apply-patch/src/text_file.rs`: unchanged lines keep their + * endings, inserted lines take the file's first ending (LF when it has none), and every line ends with + * an ending, which keeps apply_patch's trailing-newline behavior. + */ +export class SourceText { + private constructor( + private readonly lines: readonly SourceLine[], + private readonly preferredEnding: LineEnding, + ) {} + + static parse(content: string): SourceText { + const lines: SourceLine[] = []; + let preferredEnding: LineEnding | undefined; + let lineStart = 0; + for (let cursor = 0; cursor < content.length; cursor++) { + const character = content[cursor]; + if (character !== "\n" && character !== "\r") continue; + const ending: LineEnding = character === "\r" && content[cursor + 1] === "\n" ? "\r\n" : character; + preferredEnding ??= ending; + lines.push({ text: content.slice(lineStart, cursor), ending }); + cursor += ending.length - 1; + lineStart = cursor + 1; + } + if (lineStart < content.length) lines.push({ text: content.slice(lineStart), ending: undefined }); + return new SourceText(lines, preferredEnding ?? "\n"); + } + + get texts(): string[] { + return this.lines.map((line) => line.text); + } + + replace(replacements: readonly LineReplacement[]): string { + const next: SourceLine[] = []; + let sourceIndex = 0; + for (const { start, oldLength, newLines } of [...replacements].sort((left, right) => left.start - right.start)) { + next.push(...this.lines.slice(sourceIndex, start)); + next.push(...newLines.map((text) => ({ text, ending: this.preferredEnding }))); + sourceIndex = start + oldLength; + } + next.push(...this.lines.slice(sourceIndex)); + return next.map((line) => line.text + (line.ending ?? this.preferredEnding)).join(""); + } +} diff --git a/test/line-endings.test.ts b/test/line-endings.test.ts new file mode 100644 index 0000000..6ae6382 --- /dev/null +++ b/test/line-endings.test.ts @@ -0,0 +1,130 @@ +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { applyPatchDetailed, createApplyPatchTool } from "../src/index.js"; + +const tempDirectories: string[] = []; +const identityTheme = { + fg: (_name: string, text: string) => text, + bg: (_name: string, text: string) => text, + bold: (text: string) => text, + inverse: (text: string) => text, +}; + +async function workspaceWith(name: string, content: string): Promise<{ cwd: string; file: string }> { + const cwd = await mkdtemp(path.join(tmpdir(), "pi-apply-patch-endings-")); + tempDirectories.push(cwd); + const file = path.join(cwd, name); + await writeFile(file, content); + return { cwd, file }; +} + +afterEach(async () => { + await Promise.all(tempDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true }))); +}); + +describe("apply_patch line endings", () => { + it("#given a CRLF file #when a line is changed and one inserted #then every line keeps CRLF", async () => { + // given + const { cwd, file } = await workspaceWith("lines.txt", "one\r\ntwo\r\nthree\r\n"); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: lines.txt\n@@\n-one\n+ONE\n two\n+between\n three\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(file, "utf8")).toBe("ONE\r\ntwo\r\nbetween\r\nthree\r\n"); + }); + + it("#given a file with mixed line endings #when one line changes #then the untouched lines keep their own endings", async () => { + // given + const { cwd, file } = await workspaceWith("lines.txt", "one\r\ntwo\rthree\nfour\r\n"); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: lines.txt\n@@\n one\n two\n-three\n+THREE\n four\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(file, "utf8")).toBe("one\r\ntwo\rTHREE\r\nfour\r\n"); + }); + + it("#given an LF file without a final newline #when updated #then it stays LF and gains the trailing newline", async () => { + // given + const { cwd, file } = await workspaceWith("lines.txt", "alpha\nbeta"); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: lines.txt\n@@\n-beta\n+BETA\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(file, "utf8")).toBe("alpha\nBETA\n"); + }); + + it("#given a context line matched only after trimming #when patched #then that source line is left exactly as it was", async () => { + // given + const { cwd, file } = await workspaceWith("code.ts", "const a = 1; \r\nconst b = 2;\r\n"); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: code.ts\n@@\n const a = 1;\n-const b = 2;\n+const b = 3;\n*** End Patch\n", + ); + + // then + expect(result.failures).toEqual([]); + expect(await readFile(file, "utf8")).toBe("const a = 1; \r\nconst b = 3;\r\n"); + }); + + it("#given a CRLF file #when the patch context does not match #then the failure is reported and the bytes are untouched", async () => { + // given + const original = "one\r\ntwo\r\n"; + const { cwd, file } = await workspaceWith("lines.txt", original); + + // when + const result = await applyPatchDetailed( + cwd, + "*** Begin Patch\n*** Update File: lines.txt\n@@\n-missing\n+x\n*** End Patch\n", + ); + + // then + expect(result.appliedFiles).toEqual([]); + expect(result.failures.map((failure) => failure.filePath)).toEqual(["lines.txt"]); + expect(await readFile(file, "utf8")).toBe(original); + }); + + it("#given a CRLF file #when one line changes through the tool #then the shown diff counts only that line", async () => { + // given + const { cwd, file } = await workspaceWith("sample.txt", "keep\r\nbefore\r\nkeep too\r\n"); + const patch = "*** Begin Patch\n*** Update File: sample.txt\n@@\n-before\n+after\n*** End Patch"; + const tool = createApplyPatchTool(); + + // when + const result = await tool.execute("apply-patch-crlf-preview", { input: patch }, undefined, undefined, { + cwd, + } as never); + const rendered = + tool + .renderResult?.( + result, + { expanded: true, isPartial: false }, + identityTheme as never, + { cwd, toolCallId: "apply-patch-crlf-preview", args: { input: patch } } as never, + ) + ?.render(120) + .join("\n") ?? ""; + + // then + expect(rendered).toContain("• Edited sample.txt (+1 -1)"); + expect(await readFile(file, "utf8")).toBe("keep\r\nafter\r\nkeep too\r\n"); + }); +});