Skip to content

edit: keep a CRLF file's line endings when dotenv, TOML and top-level YAML lines are added or replaced - #829

Merged
yetone merged 1 commit into
yetone:mainfrom
charan-rathore:edit-keep-crlf-text
Oct 4, 2026
Merged

yetone merged 1 commit into
yetone:mainfrom
charan-rathore:edit-keep-crlf-text

Conversation

@charan-rathore

Copy link
Copy Markdown
Contributor

Config files edited on Windows are usually saved with \r\n. SetEnvFile, the TOML setters (SetTOMLTop, SetTOMLKey, SetTOMLTable) and the top-level YAML setters build the lines they add with \n and join the file, so a CRLF file came back with a bare \n on every line the edit added or replaced:

A=1\r\nB=2\r\n   SetEnvFile A=9   ->   A=9\n B=2\r\n

The file is now written back through joinLinesLike, which turns every line break into \r\n when the original used \r\n for all of them. Untouched lines already carry their \r, so only the added lines change. A file that already mixed endings is written as before.

Tests: TestSetEnvFileKeepsCRLF, TestSetTOMLKeepsCRLF and TestSetYAMLTopKeepsCRLF fail on the old code. TestSetEnvFileKeepsMixedEndings pins that a mixed file is left alone.

Run: go vet and go test -tags nogui ./internal/edit pass on this branch, and on current main with it merged; GOOS=windows go build passes. The full go test -tags nogui -count=1 ./... (temp HOME) passes except internal/sessions TestCodexArchivedSameSizeRewriteInvalidatesDiscovery, which fails the same way on unmodified main in this environment. Not run: the GUI/WebKit tests. Prepared with AI assistance.

JSON edits have the same problem; I will send that separately.

… YAML lines are added or replaced

SetEnvFile, the TOML setters and the top-level YAML setters build the lines they add with \n, then join the whole file. In a file saved with \r\n (the usual case for a config edited on Windows) that left the new or replaced lines with a bare \n among \r\n ones. SetEnvFile turned "A=1\r\nB=2\r\n" with A set to 9 into "A=9\nB=2\r\n", and SetTOMLTop, a new TOML key or table, and SetYAMLTop did the same.

The files are written back through joinLinesLike, which turns every line break into \r\n when the original used \r\n for all of them. Untouched lines already carry their \r, so only the lines the edit added change; a file that mixed endings is written as before.

TestSetEnvFileKeepsCRLF, TestSetTOMLKeepsCRLF and TestSetYAMLTopKeepsCRLF fail on the old code; TestSetEnvFileKeepsMixedEndings pins that a mixed file is left alone. go vet and go test ./internal/edit pass on this branch and on current main with it merged; the full go test -tags nogui ./... run (under a temp HOME) passes except internal/sessions TestCodexArchivedSameSizeRewriteInvalidatesDiscovery, which fails the same way on unmodified main here.
@yetone
yetone merged commit 0c38071 into yetone:main Oct 4, 2026
8 checks passed
yetone added a commit that referenced this pull request Oct 4, 2026
… with no trailing break, in a multi-line TOML string, a YAML block scalar and after DelJSON (review of #829, #830, #831) — tests only. TestReviewLFUntouched and TestReviewCRLFEdges; the CRLF cases fail on the code before those PRs.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@yetone

yetone commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks @charan-rathore, merged and released in v0.1.915.

I pulled #829, #830 and #831 together onto main and checked them:

  • go vet passes, the darwin, linux-nogui and windows builds pass, and go test -tags nogui ./... passes under a temp HOME.
  • Your CRLF tests fail on the old code, as they should.

I also added review tests (internal/edit/crlf_review_test.go, 2bfed7a). They cover:

  • LF files coming through SetEnvFile, DelEnvFile, the TOML setters, SetYAMLTop, DelYAMLTop, SetJSON, DelJSON, SetYAML and EditYAMLStrings byte-for-byte LF;
  • a CRLF dotenv file with no trailing line break;
  • a multi-line TOML string, which keeps \r\n inside it;
  • a YAML block scalar, which still reads back as one\ntwo\n;
  • DelJSON on a CRLF file.

All of them pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants