Skip to content

fix: preserve trailing newlines in text-based 3-way merges - #2891

Open
jorrite wants to merge 5 commits into
cloudposse:mainfrom
jorrite:fix-scaffold-update-text-newline
Open

fix: preserve trailing newlines in text-based 3-way merges#2891
jorrite wants to merge 5 commits into
cloudposse:mainfrom
jorrite:fix-scaffold-update-text-newline

Conversation

@jorrite

@jorrite jorrite commented Aug 6, 2026

Copy link
Copy Markdown

what

  • Fix atmos scaffold generate --update unconditionally stripping the trailing newline from every file it 3-way-merges, whether or not the file actually changed.
  • pkg/generator/merge/text_merger.go: TextMerger.Merge() now appends one newlineSeparator ("\n") to each of ours/base/theirs before handing them to diff3.Merge, so diff3's guaranteed loss of exactly one trailing newline cancels out and the original count survives.
  • pkg/generator/merge/text_merger_test.go: consolidated the trailing-newline regression coverage into a single table-driven test, TestTextMerger_TrailingNewlinePreservation, asserting exact byte-for-byte output across a no-op merge (0/1/2/3 trailing newlines, plus an internal blank line) and a genuine template change (theirs with 0/1/2 trailing newlines).
  • No change to conflict detection, threshold behavior, or ConflictStrategy handling — out of scope, and unaffected since the appended newline is identical across all three inputs.

why

  • TextMerger.Merge() delegates the actual 3-way merge to epiclabs-io/diff3, which reads each of base/ours/theirs line-by-line via bufio.Scanner (ScanLines, Go's standard-library default split function) and rejoins the merged lines with strings.Join(lines, "\n").
  • ScanLines strips every line's terminator — including the last — and gives no way to tell afterward whether the original input ended with a trailing newline or not. Concretely: for content ending in N trailing newlines, the round-trip through GetLines + Join always reconstructs exactly N-1 (it loses exactly one, regardless of how many there were; for N = 0 there was nothing to lose in the first place). Verified directly: generating a file with 3 trailing newlines and running --update with nothing changed on the template side reproducibly comes back with 2.
  • Appending one newline to each input before the merge bumps every input's count to at least 1, so that guaranteed loss of exactly one cancels out and the original count is preserved — for both the no-op case and genuine changes, since whichever side's content ends up dominating a given region carries its own (now-restored) newline count through, independent of the others.
  • This must be applied to all three inputs, not just theirs: appending it only to theirs makes an otherwise-identical ours/theirs pair (a very common no-op shape) differ by one trailing newline as far as diff3 is concerned, which turns a no-op into a spurious detected change/conflict instead of fixing anything.

references

Summary by CodeRabbit

  • Bug Fixes

    • Improved text merging to preserve the exact number of trailing newlines.
    • Prevented unintended changes to blank lines and end-of-file newline states during conflict-free merges.
    • Ensured merged content remains byte-for-byte consistent when no substantive changes are made.
  • Tests

    • Added coverage for merges involving varying trailing-newline counts, blank lines, and template-only changes.

jorrite added 3 commits August 6, 2026 16:15
TextMerger.Merge() delegates to epiclabs-io/diff3, whose line reader strips
every line's trailing newline (via bufio.Scanner) and whose join step never
restores one - so a genuinely unchanged file (and any real edit) lost its
trailing newline unconditionally on `atmos scaffold generate --update`.

matchTrailingNewline() now restores the reference's (theirs') exact
trailing-newline count rather than a binary "append one \n" - this also
correctly handles a blank line at EOF (2+ trailing newlines), and makes a
separate no-op short-circuit unnecessary: when nothing meaningful changed,
ours == theirs, so matching theirs' newlines already reconstructs ours
byte-for-byte.
…fore passing to diff3 to let it cancel the separator we just added
@jorrite
jorrite requested a review from a team as a code owner August 6, 2026 15:52
@atmos-pro

atmos-pro Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Tip

Atmos Pro  

No affected stacks workflow was detected for this pull request.
If this is expected, no action is needed.
Learn More. Ask AI.

@mergify mergify Bot added the triage Needs triage label Aug 6, 2026
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ed3de7e4-bf49-4370-99fd-c3a0bf2dc82f

📥 Commits

Reviewing files that changed from the base of the PR and between ecfc8db and 1e9df24.

📒 Files selected for processing (2)
  • pkg/generator/merge/text_merger.go
  • pkg/generator/merge/text_merger_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/generator/merge/text_merger.go
  • pkg/generator/merge/text_merger_test.go

📝 Walkthrough

Walkthrough

TextMerger.Merge now preserves trailing-newline counts by appending a newline to each diff3 input. Table-driven tests verify byte-exact output for unchanged and template-only merges.

Changes

Trailing-newline preservation

Layer / File(s) Summary
Text merge and validation
pkg/generator/merge/text_merger.go, pkg/generator/merge/text_merger_test.go
TextMerger.Merge appends a newline to each diff3 input. Tests cover missing, single, and multiple trailing newlines, blank lines, unchanged inputs, and template-only changes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • #2886: The merge changes address reported blank-line collapse during scaffold updates.

Suggested labels: patch

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fix for preserving trailing newlines in text-based three-way merges.
Linked Issues check ✅ Passed The implementation and regression tests address issue #2887 by preserving trailing newlines during non-YAML text merges.
Out of Scope Changes check ✅ Passed The code and test changes are limited to trailing-newline preservation in text-based three-way merges.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/generator/merge/text_merger_test.go`:
- Around line 820-839: Extend the merge test cases around the existing
genuine-change scenarios to cover template-only EOF newline changes: keep base
and ours identical, and verify theirs both adds and removes trailing newlines,
including “line 1\n” to “line 1\n\n” and “line 1”. Set each expected result to
preserve theirs’ newline count.
- Around line 756-761: Add a Go doc comment beginning with “Merge” immediately
before the exported TextMerger.Merge method, documenting its purpose and the
newline behavior referenced by TestTextMerger_TrailingNewlinePreservation. Keep
the existing test comment unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a8cfbfe6-71e9-4064-8143-67c721c93c00

📥 Commits

Reviewing files that changed from the base of the PR and between 3ce4349 and ecfc8db.

📒 Files selected for processing (2)
  • pkg/generator/merge/text_merger.go
  • pkg/generator/merge/text_merger_test.go

Comment thread pkg/generator/merge/text_merger_test.go
Comment thread pkg/generator/merge/text_merger_test.go
@github-actions github-actions Bot added the size/m Medium size PR label Aug 6, 2026
@mergify mergify Bot removed the triage Needs triage label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m Medium size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scaffold generate --update strips trailing newlines (if any) from every merged file

1 participant