Fix WriteLinesToFile rewriting unchanged file when custom encoding is used

[huulinhnguyen-dev]

Fixes #14071

Context

WriteLinesToFile with WriteOnlyWhenDifferent="true" is supposed to skip the write (and preserve the file timestamp) when the content it would produce is identical to what's already on disk. This keeps incremental builds fast and avoids unnecessary timestamp churn that triggers downstream rebuilds.

This skip logic was broken whenever a custom Encoding was specified (utf-8, unicode, utf-32, etc.): the file was rewritten on every build even when nothing changed, so any target depending on its timestamp re-ran each time.

There were two compounding root causes in FilesAreIdentical:

1. Wrong encoding for comparison — it encoded the candidate content with the default UTF-8 (no-BOM) encoding (s_defaultEncoding) instead of the encoding the task actually writes the file with.
2. Preamble (BOM) ignored — File.WriteAllText prepends encoding.GetPreamble() (the BOM) to the file, but the comparison never accounted for those bytes.

Because of (1) and (2), the byte length and/or content never matched the on-disk file, so it was always treated as "different" and rewritten.

Changes Made

• Thread the real encoding through the call chain: Execute → ShouldWriteFileForOverwrite → FilesAreIdentical.
• In FilesAreIdentical, encode the candidate content with that encoding and include encoding.GetPreamble() in both the length check and the byte-by-byte comparison.
• Kept the existing streamed/chunked comparison (no loading the whole file into memory). s_defaultEncoding is still used as the default when no Encoding is supplied — no dead code.

Testing

Notes

[Copilot]

Pull request overview

This PR fixes WriteLinesToFile’s WriteOnlyWhenDifferent="true" behavior when a custom Encoding is specified, ensuring the task correctly detects unchanged output and avoids rewriting the file (preserving timestamps and incremental build correctness).

Changes:

• Thread the resolved Encoding through Execute → ShouldWriteFileForOverwrite → FilesAreIdentical.
• Update FilesAreIdentical to include the encoding preamble (BOM) in both length and byte-by-byte comparisons.
• Add a regression test covering multiple BOM-emitting encodings.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Tasks/FileIO/WriteLinesToFile.cs	Use the actual task encoding (including BOM) when determining whether an overwrite would change file bytes.
src/Tasks.UnitTests/WriteLinesToFile_Tests.cs	Add regression coverage for WriteOnlyWhenDifferent with custom encodings.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AlesProkop]

/review

[AlesProkop]

Expert review (ultimate-reviewer · portable core + dotnet/msbuild expert-reviewer)

Verdict: LGTM — 0 blocking, 0 major, 0 moderate, 1 optional nit. 12 of 13 evaluated dimensions clean. Considered the prior conversation: the 3 earlier Copilot inline concerns are all resolved by the simplification in 5d5f9ce.

Prior review concerns — all resolved

• long used as an array index (would not compile) → gone; content is now indexed with int contentOffset.
• int overflow in preamble.Length + newContentBytes.Length → fixed with (long) cast.
• checked((int)(fileOffset + i)) could throw and be swallowed → removed; the per-byte checked/long scheme was replaced by comparing the preamble once up front, then a simple int offset over the content.

Why the fix is correct (verified)

• Both write paths use File.WriteAllText(..., encoding) — the non-transactional path (ExecuteNonTransactional) and the transactional path (SaveAtomically, ChangeWave 18.3) — so the compare in FilesAreIdentical and the actual write always use the same encoding object and therefore the same GetPreamble().
• Empirically confirmed on .NET that encoding.GetPreamble() exactly matches the BOM bytes File.WriteAllText prepends for utf-8, unicode, and utf-32 — including the empty-content case (e.g. utf-8 writes a 3-byte file for ""), so expectedLength = preamble + content holds even for empty output.
• Bounds are safe: after the length check, the bytes remaining past the preamble equal newContentBytes.Length exactly, so contentOffset + i never overruns. contentOffset as int is fine because a byte array length is bounded by int.MaxValue.
• Default (no Encoding) path is unchanged: s_defaultEncoding is UTF8Encoding(emitBOM: false), so its preamble is empty — no regression for the common case.

Compatibility / ChangeWave

No gate needed. This restores the documented WriteOnlyWhenDifferent contract (skip + preserve timestamp on identical content). It makes the build do less work on unchanged, byte-identical output, so there is no stale-output risk and no new warning/error. The transactional write path is already behind ChangeWave 18.3; the comparison fix applies correctly to both paths.

Clean dimensions

Correctness/edge cases, Backward-compat/ChangeWave, Concurrency ([MSBuildMultiThreadableTask]; no new shared state; FileShare.Read), Error handling (reuses existing resources; unreadable → treated as different → safe rewrite), Performance (streamed/chunked compare preserved; buffered ReadByte; no whole-file load), Design, Portability (byte-level compare), Documentation (<remarks> explains why the BOM matters), Simplicity/Naming, Observability, Change hygiene.

Optional nit — test coverage

src/Tasks.UnitTests/WriteLinesToFile_Tests.cs (WriteOnlyWhenDifferentRespectsEncoding) asserts only the timestamp before/after. Optionally, after the a3 rewrite, also assert the bytes round-trip in the target encoding, e.g. File.ReadAllText(file, System.Text.Encoding.GetEncoding(encoding)) equals "File contents2" + Environment.NewLine. Not required — the skip behavior (the actual bug) is already well covered, and this test would fail against the pre-fix code.

_{Automated review via the ultimate-reviewer skill. Findings are advisory.}