diff --git a/README.md b/README.md index 413d3d6..99f047b 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ [![CI](https://github.com/amrali-eg/EncodingChecker/actions/workflows/ci.yml/badge.svg)](https://github.com/amrali-eg/EncodingChecker/actions/workflows/ci.yml) -# EncodingChecker v3.11.2 +# EncodingChecker v3.12.0 EncodingChecker is a Windows tool for finding, checking, and safely converting text-file encodings. Use the GUI for everyday work or the command line for repeatable jobs. diff --git a/docs/CLI.md b/docs/CLI.md index 1c3f7d7..2cc4ed6 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -40,13 +40,22 @@ conversion of the same folder will rewrite them like anything else. Keep exporte plans, journals, and reports outside the folder you scan. Common metadata and build folders such as `.git`, `bin`, `obj`, and -`node_modules` are skipped. Hidden, system, and reparse-point files are left -alone, and hidden, system, and reparse-point folders are not entered. - -EC reports how many files each exclusion skipped, counting only files your -patterns actually selected — so `-Include "*.bak"` reports that they were -skipped instead of returning nothing at all. These counts do not change the -exit code. +`node_modules` are skipped, and no pattern reaches into them. Hidden, system, +and reparse-point files are left alone, and hidden, system, and reparse-point +folders are not entered. + +EC reports what each exclusion skipped, so a clean result cannot stand in for +complete coverage. For files it reports counts, limited to files your patterns +actually selected — so `-Include "*.bak"` reports that they were skipped instead +of returning nothing at all. For skipped folders it reports the folders, not +their contents, because it does not walk them to find out. + +A folder EC tried to read and could not — a permission denial, or one removed +mid-scan — is reported the same way, counted separately from the folders it +skipped on purpose. **These counts do not change the exit code.** A run that +could not read part of the tree still exits 0, so a script that must not pass +over unexamined content has to read the counts on stderr rather than the exit +code alone. ## Conversion @@ -54,7 +63,7 @@ exit code. |---|---| | `-Target ` | Target encoding, for example `utf-8` or `utf-8-bom`. Required for conversion. | | `-From ` | Explicit original encoding for every selected file. Use when you know a legacy source encoding. | -| `-Backup` | Save every replaced original as `.bak`. | +| `-Backup` | Save the replaced original as `.bak`. The name is fixed, so converting the same file again replaces that backup. | | `-WhatIf` | Show a one-time preview without writing files. | | `-Plan ` | Write a reviewable conversion plan; do not modify files. | | `-Apply ` | Execute a saved plan. Its scope and conversion settings are fixed. | @@ -101,7 +110,7 @@ combined with conversion options. | `-Journal ` | Write a JSON record of the conversion decision and final result for every file. Convert mode only. | | `-Quiet` | Suppress per-file CSV and normal summaries. Errors and coverage warnings still go to stderr. | | `-Verbose` | Include error details and a result breakdown. | -| `-MaxParallelism ` | Maximum simultaneous files. Default: the smaller of CPU count and 4. | +| `-MaxParallelism ` | Maximum simultaneous files. Default: the smaller of CPU count and 8. | `-Quiet` and `-Verbose` cannot be combined. diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index d780bf5..6c73f1f 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -3,9 +3,10 @@ Status of the thirty-five findings from the two independent reviews that preceded v3.11.0, plus what has been found since. -**Of the original thirty-five: 26 fixed, 7 open, 2 could not be reproduced.** +**Of the original thirty-five: 26 fixed, 8 open, 1 could not be reproduced.** Six further findings have been raised since v3.11.1, two of them already fixed. -**Eleven open in total.** +Twelve more since v3.11.2, all twelve fixed. +**Twelve open in total.** ## Why this file exists @@ -43,7 +44,7 @@ report. | EC-05 | A plan holding an unreadable file can never be applied | fixed | A hash is required only for `Convert` entries. | | EC-06 | A drive-root base path makes every plan unusable | fixed | Fixed 2026-09-04. **Shipped broken in v3.11.0 and v3.11.1.** | | EC-07 | The refusal advises the very encoding it cannot justify | fixed | `DescribeRefusal` offers both byte orders. | -| EC-08 | An include pattern can hang the scan indefinitely | *not reproduced* | A pathological mask against a matching filename completed inside a 10 s budget. No match timeout was added, so this is not *proven fixed*. | +| EC-08 | An include pattern can hang the scan indefinitely | **open** | Reproduced 2026-09-06, but only against inputs built for it. `CompilePatterns` still emits `RegexOptions.Compiled` with `Regex.InfiniteMatchTimeout`. The 2026-09-04 attempt used a *matching* filename, which stops at the first success and cannot show it. Scored below. | | EC-09 | `.bak` files are excluded, uncounted, and unreported | fixed | `TraversalCounters.FilesExcludedAsEcArtifact`. | | EC-10 | A scan failure is journaled as a refusal | fixed | A failed snapshot is recorded as `Error`, not `Refused`. | | EC-11 | `ApplyPlan` leaks its Ctrl+C handler onto a disposed token source | fixed | Both handler sites unsubscribe in a `finally`. | @@ -82,6 +83,49 @@ report. | A ticked file can be dropped from a source choice in silence | fixed | Each row in the review's refused list carries its resolved path. `TickedFiles()` filters out rows whose path is null and says nothing, so a file the user ticked is left refused with no message. This was live until EC-06 was fixed: with a drive-root base directory every row resolved to null, so choosing an encoding reported "Conversion cancelled. No files were modified." The trigger is gone; the silent drop is not. | | Force-closing during a run can throw on the way out | **open** | The second close request abandons a run deliberately, which is correct. But the worker may then marshal its next confirmation to a form that no longer exists, and the completion handler runs against disposed controls. An error dialog at exit rather than lost work — finished files are installed and the one in flight is untouched. Reasoned from the code, not reproduced: it needs precise timing. | +## Found after v3.11.2 + +An independent review of 74d5b3d, run from the source rather than from this file. +Everything it found and fixed is below, one commit each, carrying that commit's +measurement and mutation result. Ordered by what a reader needs first: what EC +did to files, then what it reported, then what it documented. + +| Finding | Status | Note | +|---|---|---| +| "Already in the target encoding" was decided by label, not by codec | fixed | `Decide` compared the detected charset's `WebName` against whatever the caller typed, so every accepted alias for one code page failed the test. `-Target unicode`, `ucs-2` or `utf-16le` on a tree already in UTF-16LE decoded, re-encoded, verified and reinstalled every file to produce **identical bytes**, resetting every modification time and, with `-Backup`, leaving a `.bak` and an `.ecmeta.json` beside each. Under `-FailOnChanges` the same tree exits 0 for `-Target utf-16` and 2 for `-Target unicode`; on BOM-less UTF-16 the alias reaches the ambiguity guard and exits 5, so the spelling alone moved a clean run to a refusal. Now compares resolved code pages; a zero code page proves nothing and never matches. ASCII to UTF-8 was deliberately left a conversion, on reasoning the 64 KiB finding below then disproved — folding them together is now safe and is not yet done. | +| "Already in the target encoding" was a whole-file claim made from a 64 KiB sample | fixed | Detection reads at most 64 KiB, and when the source codec matched the target nothing read further. A file clean for 64 KiB and invalid afterwards was reported `Unchanged`, and whether EC noticed depended only on which target was named: the same corrupt file was `Error` under `-Target utf-16` and `Unchanged` under `-Target utf-8`. `-Validate` always read the whole file; Convert never reached that check once it had decided it had nothing to do. Costs almost nothing, and not for the expected reason — Convert already reads every byte, because `CaptureSourceSnapshot` hashes the whole stream before anything is decided. Measured at 0.04–0.10 ms per MiB. | +| A preview promised conversions that would fail, and a plan recorded them as approved | fixed | `ApplyConversion` returned at the `whatIf` branch before the converter ran, so nothing decoded the file. `-Plan` sets `WhatIf`, so a plan recorded `Action=Convert` with no reason for a source that cannot be read, exited 0, and showed the reviewer nothing; the failure surfaced at `-Apply`, after approval and part-way through the batch. `FindStaleFiles` can prove the bytes have not changed since review and cannot prove they are readable, because that needs a decode nobody performed. The entry is now marked `Refuse`, not merely given an error, because the plan records the *action*. Decode only: a target that cannot represent the text still fails at conversion time, which reading the source cannot predict, and a test named for that case pins the limit. | +| One file's failure could end the whole run | fixed | The per-item catch in `RunParallel` named four exception types. Anything else — a `SecurityException` from an ACL the enumerator did not surface, a regex timeout, a defect in EC itself — escaped `Parallel.ForEach` as an `AggregateException` and took every file the run had not reached with it. The CLI's outer catch names the same four, so it would have surfaced as a crash rather than exit 3. Now everything except cancellation and `OutOfMemoryException`, since carrying on after the latter would be pretending to process. `RunParallel` became internal so the isolation could be tested at all: no file can be made to throw the exceptions that mattered, which is exactly what made them dangerous. | +| A folder EC could not read left no trace a machine could see | fixed | An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An unreadable *directory* produced a warning on stderr and nothing else: no row, no counter, exit 0 — and nothing whatever in the window, which passes no warning callback. Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one denied folder reported "1 file(s) processed" and exited 0, and a scan whose entire base directory was unreadable printed a header-only CSV and exited 0. A run that examined none of the tree could report success. `DirectoriesUnreadable` is now counted at both catch blocks, apart from the two exclusion counters, which record folders EC *chose* not to enter. The exit code is deliberately unchanged and the documentation now says what that means for a script. | +| Folders skipped by name were counted nowhere | fixed | Twelve directory names are skipped deliberately and that is documented, but unlike attribute-excluded folders they incremented no counter. A scan of a tree whose only content sat under `build/` reported one file, zero exclusions and no warning — while `docs/CLI.md` promised that EC reports how many files each exclusion skipped. Counted separately from the attribute exclusions, whose message says "(hidden, system, or reparse point)" and would become untrue if the two were merged. What is scanned is unchanged: letting an explicit include reach into these folders was considered and declined, because both documents state they are skipped. | +| `-Validate` rejections could carry no reason at all | fixed | Four ways to return `Invalid`, two of them explained. A charset outside the allowed list and a file EC could not identify both arrived as a bare `Invalid` with an empty reason, though they are not the same situation: one means widen the list or convert the file, the other means EC could not tell what it is, which `-DetectOnly` already calls `UnknownEncoding`. `CharsetNotAllowed` is new; `UnknownEncoding` is reused deliberately, because two names for one condition depending on which mode ran would be its own defect. This was the only outcome in the product where the reader had to re-derive a reason the producer already knew. | +| A refusal's reason was re-derived instead of read from the decision | fixed | `ApplyConversion` worked the reason code out again from the four raw facts `Decide` had already reduced to a `SourceInterpretation`. The two copies were textually identical and their operands never changed between them, so they could not disagree — but nothing tied them together, and a fourth refusal reason added to the policy would have fallen through to `LegacySourceRequired` at the call site: a correct refusal carrying the wrong explanation, with nothing to fail. That shape had already needed one bolt-on `when` guard. Now `ConversionPolicy.ReasonCodeFor`, verified equivalent across all 256 reachable combinations of `Decide`'s inputs, with a test that fails if any refusal ever produces no reason. | +| A decode failure reported a position no file has | fixed | `DecoderFallbackException.Index` is relative to the decoder call, not the file, and goes negative when the bad sequence began in bytes carried over from the previous call. A UTF-8 file ending in a truncated three-byte sequence produced "offset -2 within the failing read chunk", a message naming a frame it did not describe. The offending bytes are reported instead, which mean the same thing wherever the failure happened. An absolute file offset would need the streaming loop restructured to keep each chunk's base position in scope. | +| Standard output was UTF-8 whatever the console was | fixed | After attaching to the parent console, both writers were rebuilt with `StreamWriter`'s default encoding. On the machine this was found on `Console.OutputEncoding` is `ibm437`, and the per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen" — the tool producing in its own output the failure it exists to detect. A redirected stream stays UTF-8, matching the `-Report` file apart from its BOM; a console gets its own encoding, so characters it cannot represent become "?", which is visibly lossy rather than quietly wrong. No global console state is mutated. | +| The documented parallelism default was the old one | fixed | `DefaultMaxParallelism` was raised from `min(CPU, 4)` to `min(CPU, 8)` with the measurement recorded beside it, and both statements of it were left saying 4: the built-in help and `docs/CLI.md`, which are the two places someone tuning `-MaxParallelism` against a slow share would look. The cause was an unnamed literal, with no identity a document could be checked against; it is now `ScanEngine.MaxParallelismCap`. A test finds the one line in each document that states the default, extracts every run of digits from it, and asserts the set equals the cap, so a stale number cannot hide beside a fresh one. | +| The lifetime of `.bak` was undocumented | fixed | `.bak` is a fixed name holding the version the most recent run replaced, so converting the same file again replaces it and removes its sidecar. That is deliberate, and pinned by `BackupIntegrityTests.Backup_OverwritesAnyPreviousBackupFile` since the first commit of the test suite — but no document said so, and a user converting twice lost the original with nothing having warned them. Raised in review as a defect and **withdrawn**: refusing to overwrite a non-matching `.bak` breaks four existing tests and would block an ordinary "wrong target, convert again" run until the user deleted the backups by hand. See CX-02, whose fix accepted the replacement and removed the stale record instead. | + +### From the same review, and not tracked here + +Four findings the same review left open are recorded nowhere else in this file. +The first is the one worth reading: + +- **A BOM-less UTF-16 file can be detected as UTF-32 and converted.** Silent, and + output verification cannot catch it, because both sides of the comparison use + the same wrong codec. It needs a file in which every other UTF-16 code unit is + a C0 control — one character per line with LF endings, say. Measured over + nineteen realistic file shapes: 41 of 44 detect correctly, and the three that + do not are the same degenerate shape. Scored Critical impact, low reach. +- ASCII text with 2.3% or more NUL bytes is labelled `utf-16`. Conversion is + refused by the ambiguity guard in every case constructed, so the wrong label + reaches `-DetectOnly` and `-Validate` only. +- Two hard links to one file are converted twice, once per name. Both runs + succeeded when tested, because `File.Replace` breaks the link; the `File.Move` + fallback would not. +- Detection accepts a truncated trailing sequence, because it decodes without + flushing, while conversion flushes and rejects it. `-DetectOnly` can therefore + bless a file conversion refuses. + ## Hashing: three optimisations measured and rejected Conversion looked as though it hashed the same bytes several times over. Three @@ -212,6 +256,7 @@ trips over is not noise. |---|---|---|---|---| | CX-06 | Entropy gate outranks a valid BOM | Medium | Occasional | EC reports the wrong encoding for a file that says what it is. The only open item that changes what EC tells you. | | — | Ambiguous BOM-less UTF-32 converts silently | **Critical** | **Theoretical** | Rewrites on an unproven byte order — the exact thing this release line exists to prevent. Needs every scalar to be a multiple of 0x100, so real text will not reach it. Scored high on impact and dismissed on reach, deliberately. | +| EC-08 | An include pattern can hang the scan indefinitely | Medium | **Theoretical** | Availability, not data: a scan no token can cancel, and if it hangs partway through a conversion the tree is left partly converted with no journal. Needs *both* halves built on purpose — a mask of ~10+ wildcards separated by one character, and a filename carrying ~24+ mostly consecutive repeats of that same character. Measured at twelve wildcards: every realistic name answered in 0–5 ms; forty consecutive `a` took >20 s. The mask comes from the operator's own command line, so there is no untrusted path. Fix measured and not taken: see below. | | — | CSV report does not neutralise leading formula characters | Medium | Rare | Needs an attacker-influenced filename and a reader who opens the report in a spreadsheet. | | EC-16 | Settings.xml written truncate-in-place | Low | Occasional | Loses preferences, not data, and reverts toward safer defaults. Already caused one smoke-test failure that looked like a product bug. | | EC-20 | Detection reads with looser file sharing | Low | Rare | Detect and validate only; nothing is written. Can describe bytes another process is changing. | @@ -225,9 +270,25 @@ trips over is not noise. Nothing here writes to a file nobody approved, which is why none of it blocked a release. +### EC-08: the fix that was measured and not taken + +`RegexOptions.NonBacktracking` in place of `Compiled` removes the blow-up +entirely, agrees with the current engine on every mask tested, and costs nothing +measurable beside per-file I/O. It was implemented with tests and then reverted +deliberately: the defect needs a mask *and* a filename both built for it, and +neither arrives from anywhere but the operator's own hands. + +Three dead ends, recorded so nobody walks them again. Testing a pathological +mask against a **matching** filename proves nothing, because the engine stops at +the first success — that is how this was first recorded "not reproduced". `*` +crossing directory separators is deliberate rather than a Windows-wildcard bug: +`src/*.cs` is meant to scope a subtree, pinned by +`PathAwarePatternTests.PathQualifiedPattern_MatchesOnlyTheIntendedSubtree`. And +`[^/]*` in place of `.*` does not reduce the backtracking, because a filename +contains no separator for it to bound. + ### Not reproduced | | Finding | Why it is not listed as open | |---|---|---| -| EC-08 | An include pattern can hang the scan indefinitely | A pathological mask completed inside a 10 s budget. No match timeout was added, so this is not *proven fixed* either. | | EC-19 | The double-BOM guard's reach depends on which object supplied the codec | An inspection-only finding that traced the wrong object. `ConvertFiles` re-resolves the codec by name through `Encoding.GetEncoding`, which carries a 3-byte preamble, so the detector's BOM-less instance never reaches the guard. Tested against a file beginning with two BOMs: both the automatic path and `-From utf-8` refuse with `MultipleLeadingByteOrderMarks`. | diff --git a/docs/RELEASE-NOTES-v3.12.0.md b/docs/RELEASE-NOTES-v3.12.0.md new file mode 100644 index 0000000..e22ab36 --- /dev/null +++ b/docs/RELEASE-NOTES-v3.12.0.md @@ -0,0 +1,204 @@ +# EncodingChecker v3.12.0 + +Twelve findings from an independent review, run against the source rather than +against the backlog: a target's spelling could rewrite an entire tree, two +claims were made without reading the file, and three failures were reported to +nobody. + +## A file already in the target encoding is left alone + +`ConversionPolicy` compared the detected charset's label against whatever the +caller typed. "utf-16", "unicode", "ucs-2" and "utf-16le" all name code page +1200, so every accepted alias but one failed the test: `-Target unicode` on a +tree already in UTF-16LE decoded, re-encoded, verified and reinstalled every +file to produce **identical bytes**. + +Nothing was corrupted. What was lost was every modification time — and with +`-Backup`, a `.bak` and an `.ecmeta.json` appeared beside each unchanged file. +Under `-FailOnChanges` the same clean tree exits 0 for `-Target utf-16` and 2 +for `-Target unicode`; on BOM-less UTF-16 the alias reaches the ambiguity guard +and exits 5, so the spelling alone turned a clean run into a refusal. + +Identity is now the resolved code page. A zero code page means the label did not +resolve, so it proves nothing and never matches. + +## "Already in the target encoding" is a whole-file claim again + +Detection reads at most 64 KiB. When the source codec matched the target, +nothing read any further — so a file clean for 64 KiB and invalid afterwards was +reported `Unchanged`, and whether EC noticed depended only on which target was +named. The same corrupt file was `Error` under `-Target utf-16` and `Unchanged` +under `-Target utf-8`. `-Validate` always read the whole file; Convert never +reached that check once it had decided it had nothing to do. + +Reading the rest costs almost nothing, and not for the reason expected: Convert +already reads every byte, because the source snapshot hashes the whole stream +before anything is decided. Measured at 0.04-0.10 ms per MiB. + +## A preview no longer promises a conversion that would fail + +`-Plan` sets `WhatIf`, and conversion returned at the `whatIf` branch before the +converter ran, so nothing decoded the file. A plan therefore recorded +`Action=Convert`, with no reason, for a source that cannot be read; the run +exited 0 and showed the reviewer nothing. The failure surfaced at `-Apply`, +after approval and part-way through the batch. + +A staleness check cannot substitute. It proves the bytes have not changed since +review and cannot prove they are readable, because that needs a decode nobody +performed. The entry is now marked `Refuse` rather than merely given an error, +because a plan records the *action*. + +Decode only. A target that cannot represent the text still fails at conversion +time, which reading the source cannot predict, and a test named for that case +pins the limit. + +## One file's failure is one row, whatever it threw + +The per-item catch named four exception types. Anything else — a +`SecurityException` from an ACL the enumerator did not surface, a regex timeout, +a defect in EC itself — escaped `Parallel.ForEach` as an `AggregateException` +and took every file the run had not yet reached with it. The CLI's outer catch +names the same four, so it would have surfaced as a crash rather than exit 3. + +It now catches everything except cancellation and `OutOfMemoryException`, since +carrying on after the latter would be pretending to process rather than +processing. + +## A folder EC could not read leaves a trace + +An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An +unreadable *directory* produced a warning on stderr and nothing else: no row, no +counter, exit 0 — and nothing whatever in the window, which passes no warning +callback. + +Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one +denied folder reported "1 file(s) processed" and exited 0, and a scan whose +entire base directory was unreadable printed a header-only CSV and exited 0. A +run that examined none of the tree could report success. + +Unreadable directories are now counted, separately from the two exclusion +counters, which record folders EC *chose* not to enter. The exit code is +deliberately unchanged, and the documentation now says what that means for a +script. + +## Folders skipped by name are counted + +Twelve directory names are skipped deliberately and that is documented, but +unlike attribute-excluded folders they incremented no counter. A scan of a tree +whose only content sat under `build/` reported one file, zero exclusions and no +warning, while `docs/CLI.md` promised that EC reports how many files each +exclusion skipped. + +They are counted apart from the attribute exclusions, whose message says +"(hidden, system, or reparse point)" and would become untrue if the two were +merged. What is scanned is unchanged. + +## Every `-Validate` rejection carries a reason + +There were four ways to return `Invalid` and two of them were explained. A +charset outside the allowed list and a file EC could not identify both arrived +as a bare `Invalid` with an empty reason, though they are not the same +situation: one means widen the list or convert the file, the other means EC +could not tell what the file is. + +`CharsetNotAllowed` is new. `UnknownEncoding` is reused deliberately, because +two names for one condition depending on which mode ran would be its own defect. +This was the only outcome left in the product where the reader had to re-derive +a reason the producer already knew. + +## A refusal's reason comes from the decision that made it + +The conversion path worked the reason code out again from the four raw facts the +policy had already reduced to a single interpretation. The two copies were +textually identical and their operands never changed between them, so they could +not disagree — but nothing tied them together, and a fourth refusal reason added +to the policy would have fallen through to `LegacySourceRequired` at the call +site: a correct refusal carrying the wrong explanation, with nothing to fail. + +It is now `ConversionPolicy.ReasonCodeFor`, verified equivalent across all 256 +reachable combinations of the policy's inputs, with a test that fails if any +refusal ever produces no reason. + +## A decode failure names the bytes, not a position no file has + +`DecoderFallbackException.Index` is relative to the decoder call, not to the +file, and goes negative when the bad sequence began in bytes carried over from +the previous call. A UTF-8 file ending in a truncated three-byte sequence +produced "offset -2 within the failing read chunk" — a message naming a frame it +did not describe. + +The offending bytes are reported instead, which mean the same thing wherever the +failure happened. + +## Standard output is encoded for whoever is going to read it + +After attaching to the parent console, both writers were rebuilt with +`StreamWriter`'s default encoding, which is UTF-8 whatever the console is. On +the machine this was found on, `Console.OutputEncoding` is `ibm437`, and the +per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen" — the tool +producing in its own output the failure it exists to detect. + +A redirected stream stays UTF-8, matching the `-Report` file apart from its BOM. +A console gets its own encoding, so characters it cannot represent become "?", +which is visibly lossy rather than quietly wrong. No global console state is +mutated. + +## The documented parallelism default is the one the code uses + +v3.11.2 raised the cap from `min(CPU, 4)` to `min(CPU, 8)` and left both +statements of it saying 4: the built-in help and `docs/CLI.md`, which are the +two places someone tuning `-MaxParallelism` against a slow share would look. The +cause was an unnamed literal, with no identity a document could be checked +against; it is now `ScanEngine.MaxParallelismCap`. A test finds the one line in +each document that states the default, extracts every run of digits from it, and +asserts the set equals the cap, so a stale number cannot hide beside a fresh one. + +## Documentation + +`.bak` is a fixed name holding the version the most recent run replaced, +so converting the same file twice replaces it. That is deliberate and has been +pinned by a test since the suite's first commit, but no document said so, and a +user converting twice lost the original with nothing having warned them. +[`docs/SAFETY.md`](SAFETY.md) now says it. + +[`docs/DEFECT-BACKLOG.md`](DEFECT-BACKLOG.md) records every finding of this +review: the twelve fixed here, one withdrawn once its fix was shown to break +four existing tests, and four left open — including a BOM-less UTF-16 file that +can be detected as UTF-32 and converted silently, which output verification +cannot catch because both sides of the comparison use the same wrong codec. It +also records EC-08 — an include pattern that can hang a scan — as reproduced +against inputs built for it, and why the first attempt to reproduce it missed. + +## Compatibility + +Conversion semantics stay at **6**, the plan schema at **5**, the journal schema +at **4**. A plan written by an earlier build still means what it meant: these +changes make EC refuse more and convert less, which the approved-decision +ceiling already permits. + +Exit codes and report contents do change, in both directions: + +| Situation | Before | After | +|---|---|---| +| `-FailOnChanges` on a tree already in an alias of the target | 2 | 0 | +| A file valid for 64 KiB and invalid afterwards | `Unchanged`, 0 | `Error`, 3 | +| `-Plan` over a source that cannot be decoded | `Convert`, 0 | `Refuse`, 3 | +| An unexpected exception during a scan | run-ending crash | one row, 3 | +| A `-Validate` row outside the allowed list | empty reason | `CharsetNotAllowed` | + +Coverage output gains two lines, for folders skipped by name and folders that +could not be read. Standard output written to a console now uses that console's +encoding rather than UTF-8; redirected output is unchanged. + +## Verification + +- 727 tests pass, none skipped; release build with no warnings +- The nine-phase GUI smoke suite gates the release, as it has since v3.11.2 +- Every fix was mutation-checked: the change reverted, the intended test + required to fail, the file restored byte-identical and confirmed by hash +- **No four-corpus audit was run, and this release is one that asks for one.** + The checklist requires a corpus run for a release changing detection or + conversion policy. Detection is untouched — no detector file changed — but + `ConversionPolicy` is not, so the exemption v3.11.2 claimed is unavailable + here. What supports this release is the unit suite, the GUI suite, the + detector parity check, and a mutation check on each fix. diff --git a/docs/SAFETY.md b/docs/SAFETY.md index 3e57b6b..704114d 100644 --- a/docs/SAFETY.md +++ b/docs/SAFETY.md @@ -68,6 +68,12 @@ whether installation was prepared or completed. The `.bak` and sidecar provide independently verifiable recovery information. EC does not currently provide a built-in restore command. +`.bak` is a fixed name holding the version the most recent run replaced, so +it is one level of undo rather than a history. Converting the same file again +replaces the backup, and removes its sidecar with it. If you may want the original +of a file you are about to convert a second time, copy the existing `.bak` +elsewhere first. + `-Journal` creates the batch-level record: detection, chosen source, decision, reason code, final result, and before/after hashes for every file. diff --git a/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs b/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs new file mode 100644 index 0000000..2c9f9b1 --- /dev/null +++ b/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs @@ -0,0 +1,62 @@ +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// Standard output must be encoded for whoever is going to read it. +/// +/// +/// After attaching to the parent console, both writers were rebuilt with +/// StreamWriter's default encoding, which is UTF-8 whatever the console is. On a +/// CP437 console — the default on the machine this was found on — the per-file CSV rendered +/// "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen": the tool's own output exhibiting the +/// defect it exists to detect. +/// +public sealed class ConsoleOutputEncodingTests +{ + private static byte[] Write(bool redirected, string text) + { + using var buffer = new MemoryStream(); + + using (StreamWriter writer = Program.OpenStandardWriter(buffer, redirected)) + writer.Write(text); + + return buffer.ToArray(); + } + + [Fact] + public void RedirectedOutputIsUtf8WithoutABom() + { + // A file or a pipe wants the encoding the -Report file uses, minus its BOM. + byte[] written = Write(redirected: true, "Gr\u00fc\u00dfe"); + + Assert.Equal(new UTF8Encoding(false).GetBytes("Gr\u00fc\u00dfe"), written); + } + + [Fact] + public void ConsoleOutputUsesTheConsoleEncoding() + { + // Whatever the console decodes with is what it must be handed. + byte[] written = Write(redirected: false, "Gr\u00fc\u00dfe"); + + Assert.Equal(Console.OutputEncoding.GetBytes("Gr\u00fc\u00dfe"), written); + } + + [Fact] + public void NeitherWriterEmitsAPreamble() + { + // A BOM in the middle of a shell session, or at the head of a piped CSV, would be + // a new defect rather than a fix. + Assert.Empty(Write(redirected: true, string.Empty)); + Assert.Empty(Write(redirected: false, string.Empty)); + } + + [Fact] + public void AsciiIsIdenticalEitherWay() + { + // The common case must not depend on which branch ran. + Assert.Equal( + Write(redirected: true, "File,Encoding,BOM"), + Write(redirected: false, "File,Encoding,BOM")); + } +} diff --git a/sources/EncodingChecker.Tests/ConversionPolicyTests.cs b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs index c6484a0..abddc23 100644 --- a/sources/EncodingChecker.Tests/ConversionPolicyTests.cs +++ b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs @@ -140,8 +140,8 @@ public void BothSurfacesReachTheSameDecisionForTheSameFile() public void DetectedLegacyTextRequiresAnExplicitSourceChoice() { PlannedAction action = ConversionPolicy.Decide( - "windows-1252", sourceHasBom: false, - "utf-16le", targetHasBom: false, + "windows-1252", sourceCodePage: 1252, sourceHasBom: false, + "utf-16le", targetCodePage: 1200, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -158,8 +158,8 @@ public void UnicodeAndExplicitSourcesCanConvert() Assert.Equal( PlannedAction.Convert, ConversionPolicy.Decide( - "utf-8", sourceHasBom: false, - "utf-16le", targetHasBom: false, + "utf-8", sourceCodePage: 65001, sourceHasBom: false, + "utf-16le", targetCodePage: 1200, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: true, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -169,8 +169,8 @@ public void UnicodeAndExplicitSourcesCanConvert() Assert.Equal( PlannedAction.Convert, ConversionPolicy.Decide( - "windows-1252", sourceHasBom: false, - "utf-8", targetHasBom: false, + "windows-1252", sourceCodePage: 1252, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: true, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -186,8 +186,8 @@ public void AFileAlreadyInTheTargetEncodingIsNotRefusedForLegacyDetection() Assert.Equal( PlannedAction.Unchanged, ConversionPolicy.Decide( - "utf-8", sourceHasBom: false, - "utf-8", targetHasBom: false, + "utf-8", sourceCodePage: 65001, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: true, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -203,8 +203,8 @@ public void AnUnidentifiedSourceIsSkippedByBothTheGuardAndThePolicy() Assert.Equal( PlannedAction.Skip, ConversionPolicy.Decide( - ScanEngine.UnknownCharset, sourceHasBom: false, - "utf-8", targetHasBom: false, + ScanEngine.UnknownCharset, sourceCodePage: 0, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, diff --git a/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs b/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs new file mode 100644 index 0000000..8fb7e14 --- /dev/null +++ b/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs @@ -0,0 +1,111 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A decode failure has to describe something the reader can find. +/// +/// +/// The diagnostic reported DecoderFallbackException.Index as "offset N within the +/// failing read chunk". That index is relative to the decoder call, not the file, and is +/// negative when the bad sequence began in bytes carried over from the previous call, so a +/// truncated tail produced "offset -2" — a position no file has, in a message naming a +/// frame it did not describe. The offending bytes are reported instead. +/// +public sealed class DecodeFailureDiagnosticTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-decode-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// + /// Converts one file, naming the source codec so the decoder is what fails. + /// + /// + /// Malformed UTF-8 is valid windows-1252, so automatic detection relabels these files + /// as legacy text and the policy refuses them before any decoding happens. Naming the + /// source is how a caller reaches the decoder with bytes it cannot accept. + /// + private ConversionReportEntry Convert(byte[] contents, string? from = null) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = "utf-16", + SourceCharset = from, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Cjk(int repeats) => + new UTF8Encoding(false).GetBytes( + string.Concat(Enumerable.Repeat("中文内容测试", repeats))); + + [Fact] + public void ATruncatedTrailingSequenceNamesTheBytes() + { + // Begins in one decoder call and fails in the flush: the case that produced the + // negative index, and the one automatic detection still calls UTF-8. + ConversionReportEntry entry = Convert([.. Cjk(3), 0xE4, 0xB8]); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(nameof(ConversionErrorCode.SourceDecodeError), entry.ReasonCode); + Assert.Contains("0xE4B8", entry.Diagnostic); + } + + [Theory] + [InlineData(new byte[] { 0xC0, 0xAF }, "0xC0")] // overlong "/" + [InlineData(new byte[] { 0x80 }, "0x80")] // lone continuation byte + [InlineData(new byte[] { 0xED, 0xA0, 0x80 }, "0xED")] // CESU-8 surrogate + public void AnInvalidSequenceMidFileNamesTheBytes(byte[] bad, string expected) + { + ConversionReportEntry entry = + Convert([.. Cjk(3), .. bad, .. Cjk(3)], from: "utf-8"); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(nameof(ConversionErrorCode.SourceDecodeError), entry.ReasonCode); + Assert.Contains(expected, entry.Diagnostic); + } + + [Fact] + public void NoDecodeDiagnosticDescribesAPositionItCannotKnow() + { + // The wording is the regression: an index relative to a decoder call was being + // presented as an offset within a frame the reader cannot locate. + foreach ((byte[] contents, string? from) in new (byte[], string?)[] + { + ([.. Cjk(3), 0xE4, 0xB8], null), + ([.. Cjk(3), 0xC0, 0xAF, .. Cjk(3)], "utf-8"), + ([.. Cjk(3), 0x80, .. Cjk(3)], "utf-8"), + }) + { + string diagnostic = Convert(contents, from).Diagnostic ?? string.Empty; + + Assert.Contains("invalid byte sequence", diagnostic); + Assert.DoesNotContain("offset", diagnostic, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("chunk", diagnostic, StringComparison.OrdinalIgnoreCase); + } + } +} diff --git a/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs b/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs new file mode 100644 index 0000000..16d1349 --- /dev/null +++ b/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs @@ -0,0 +1,105 @@ +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; + +namespace EncodingChecker.Tests; + +/// +/// The documented default parallelism must be the one the code actually uses. +/// +/// +/// was raised from 4 to 8 with a measurement +/// recorded beside it, and both statements of it — the built-in help and +/// docs/CLI.md — were left saying 4. Nothing failed, which is the problem: someone +/// tuning -MaxParallelism against a slow share was being given the wrong baseline +/// by the two places they would look. +/// +public sealed class DocumentedParallelismDefaultTests +{ + private static string RepositoryRoot([CallerFilePath] string thisFile = "") + { + string? directory = Path.GetDirectoryName(thisFile); + + while (directory is not null && + !File.Exists(Path.Combine(directory, "docs", "CLI.md"))) + { + directory = Path.GetDirectoryName(directory); + } + + Assert.True( + directory is not null, + "docs/CLI.md was not found above the test sources; this check reads it, so it " + + "has to run from a source checkout"); + + return directory!; + } + + /// The one line in a document that states the option's default. + private static string TheLineStatingTheDefault(string text, string source) + { + string[] candidates = + [ + .. text.Split('\n') + .Select(line => line.TrimEnd('\r')) + .Where(line => + line.Contains("-MaxParallelism", StringComparison.Ordinal) && + line.Contains("efault", StringComparison.Ordinal)) + ]; + + Assert.True( + candidates.Length == 1, + $"expected exactly one line in {source} stating the -MaxParallelism default, " + + $"found {candidates.Length}; the check is not looking where it thinks it is"); + + return candidates[0]; + } + + /// Every run of digits in the line, so a stale number cannot hide beside a fresh one. + private static string[] NumbersIn(string line) => + [.. Regex.Matches(line, @"\d+").Select(m => m.Value)]; + + private static string HelpText() + { + TextWriter originalOut = Console.Out; + + try + { + using var captured = new StringWriter(); + Console.SetOut(captured); + Assert.Equal(0, Program.RunConsoleMode(["--help"])); + return captured.ToString(); + } + finally + { + Console.SetOut(originalOut); + } + } + + [Fact] + public void TheBuiltInHelpStatesTheCapTheCodeUses() + { + string line = TheLineStatingTheDefault(HelpText(), "the built-in help"); + + Assert.Equal([ScanEngine.MaxParallelismCap.ToString()], NumbersIn(line)); + } + + [Fact] + public void TheCommandLineReferenceStatesTheCapTheCodeUses() + { + string reference = File.ReadAllText( + Path.Combine(RepositoryRoot(), "docs", "CLI.md")); + + string line = TheLineStatingTheDefault(reference, "docs/CLI.md"); + + Assert.Equal([ScanEngine.MaxParallelismCap.ToString()], NumbersIn(line)); + } + + [Fact] + public void TheCapIsWhatBoundsTheDefault() + { + // Ties the documented number to the value actually handed to Parallel.ForEach, + // so documenting the cap correctly cannot drift from applying it. + Assert.Equal( + Math.Min(Environment.ProcessorCount, ScanEngine.MaxParallelismCap), + ScanEngine.DefaultMaxParallelism); + } +} diff --git a/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs b/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs new file mode 100644 index 0000000..716ab1a --- /dev/null +++ b/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs @@ -0,0 +1,137 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// Folders skipped by name must appear in the coverage report. +/// +/// +/// Eleven directory names — .git .svn .hg .vs .idea bin obj node_modules packages dist +/// build target — are skipped deliberately, and that is documented. Attribute-excluded +/// folders were counted; these were not, so a scan of a tree whose only content sat under +/// build/ reported one file, zero exclusions and no warning. In a tool that reports +/// coverage precisely so a clean result cannot stand in for complete coverage, this was the +/// one hole the coverage report could not see. +/// +/// What is scanned is unchanged: these folders are still skipped, and no pattern reaches +/// into one. Only the accounting is fixed. +/// +/// +public sealed class NameExcludedFolderCoverageTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-namedirs-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private void Populate(params string[] folders) + { + foreach (string folder in folders) + { + Directory.CreateDirectory(Path.Combine(_root, folder)); + File.WriteAllText( + Path.Combine(_root, folder, "a.txt"), "content", new UTF8Encoding(false)); + } + } + + private (List Entries, DirectoryTraversal.TraversalCounters Counters) + Scan() + { + var counters = new DirectoryTraversal.TraversalCounters(); + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Detect, + Counters = counters, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return (entries.ToList(), counters); + } + + [Fact] + public void EverySkippedFolderIsCounted() + { + Populate("build", "target", "dist", "packages", "src"); + + var (entries, counters) = Scan(); + + // Unchanged: only src is walked. + Assert.Equal("src", Path.GetFileName(Path.GetDirectoryName(Assert.Single(entries).FilePath))); + + // Fixed: the other four are accounted for rather than invisible. + Assert.Equal(4, counters.DirectoriesExcludedByName); + Assert.Equal(0, counters.DirectoriesExcludedByAttribute); + } + + [Theory] + [InlineData(".git")] + [InlineData(".svn")] + [InlineData(".hg")] + [InlineData(".vs")] + [InlineData(".idea")] + [InlineData("bin")] + [InlineData("obj")] + [InlineData("node_modules")] + [InlineData("packages")] + [InlineData("dist")] + [InlineData("build")] + [InlineData("target")] + public void EachDocumentedNameIsCounted(string folder) + { + // The list in docs/CLI.md and the built-in help names all twelve; none may be + // skipped without saying so. + Populate(folder); + + Assert.Equal(1, Scan().Counters.DirectoriesExcludedByName); + } + + [Fact] + public void ANameExclusionIsReportedSeparatelyFromAnAttributeOne() + { + // Two different reasons a folder was not entered, and the report says which. + Populate("build", "hidden"); + File.SetAttributes( + Path.Combine(_root, "hidden"), + File.GetAttributes(Path.Combine(_root, "hidden")) | FileAttributes.Hidden); + + var (_, counters) = Scan(); + + Assert.Equal(1, counters.DirectoriesExcludedByName); + Assert.Equal(1, counters.DirectoriesExcludedByAttribute); + + string coverage = MainForm.FormatCoverage(counters); + + Assert.Contains("1 folder(s) not entered", coverage); + Assert.Contains("1 build/metadata folder(s) not entered", coverage); + } + + [Fact] + public void ACleanTreeStillReportsNothing() + { + // The counters must stay silent when there is genuinely nothing to disclose, + // or the report becomes noise people learn to skip. + Populate("src"); + + var (_, counters) = Scan(); + + Assert.Equal(0, counters.DirectoriesExcludedByName); + Assert.Equal(string.Empty, MainForm.FormatCoverage(counters)); + } +} diff --git a/sources/EncodingChecker.Tests/PreviewVerificationTests.cs b/sources/EncodingChecker.Tests/PreviewVerificationTests.cs new file mode 100644 index 0000000..e086319 --- /dev/null +++ b/sources/EncodingChecker.Tests/PreviewVerificationTests.cs @@ -0,0 +1,162 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A preview must not promise a conversion that would fail. +/// +/// +/// ApplyConversion returned at the whatIf branch before the converter ran, so +/// nothing decoded the file. -Plan sets WhatIf, so a plan recorded +/// Action = Convert with no reason for a source that cannot be read, exited 0, and +/// showed the reviewer nothing; the failure surfaced at -Apply, after approval and +/// part-way through the batch. FindStaleFiles can prove the bytes have not changed +/// since review and cannot prove they are readable, because that needs a decode nobody +/// performed. +/// +/// The decode only. A source that reads cleanly can still fail on a target that cannot +/// represent it — TargetEncodeError — and previewing that means running the whole +/// conversion into a discarded buffer. +/// records that limit rather than leaving it to be discovered. +/// +/// +public sealed class PreviewVerificationTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-preview-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// Clean text past the 64 KiB detection sample, optionally spoiled at the end. + private static byte[] PastTheSample(bool corrupt) + { + var text = new StringBuilder(); + + while (Encoding.UTF8.GetByteCount(text.ToString()) < 70 * 1024) + text.Append("the quick brown fox jumps over the lazy dog. "); + + byte[] clean = new UTF8Encoding(false).GetBytes(text.ToString()); + + return corrupt ? [.. clean, 0xC0, 0xAF] : clean; + } + + private ConversionReportEntry Preview(byte[] contents, string target, string? from = null) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = target, + SourceCharset = from, + WhatIf = true, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + [Theory] + [InlineData("utf-8")] + [InlineData("utf-16")] + [InlineData("us-ascii")] + public void APreviewRefusesWhatARealRunWouldRefuse(string target) + { + ConversionReportEntry entry = Preview(PastTheSample(corrupt: true), target); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.False(string.IsNullOrEmpty(entry.Diagnostic)); + } + + [Fact] + public void APlanDoesNotScheduleAFileItCannotCarryOut() + { + // The action is what the plan records, so a preview that merely reported an error + // while still saying Convert would put it in front of a reviewer as approved work. + ConversionReportEntry entry = Preview(PastTheSample(corrupt: true), "utf-16"); + + Assert.Equal(PlannedAction.Refuse, entry.Action); + Assert.False(entry.ReplacementCommitted); + + ConversionPlan plan = ConversionPlan.FromEntries( + [entry], _root, "utf-16", targetHasBom: false, + backupEnabled: false, explicitSource: null); + + PlannedFile planned = Assert.Single(plan.Files); + + Assert.Equal(PlannedAction.Refuse, planned.Action); + Assert.False(planned.NeedsSourceChoice); + Assert.Equal(0, plan.Summary.ReadyToConvert); + } + + [Theory] + [InlineData("utf-8")] + [InlineData("utf-16")] + public void AnOrdinaryFileStillPreviewsAsConvertible(string target) + { + // The check must not start refusing the files a preview exists to describe, and + // 70 KiB puts the clean tail well past the detection sample. + ConversionReportEntry entry = Preview(PastTheSample(corrupt: false), target); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal(ConversionRowResult.Converted, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void APreviewStillWritesNothing() + { + byte[] contents = PastTheSample(corrupt: false); + + Preview(contents, "utf-16"); + + Assert.Equal(contents, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + Assert.Empty(Directory.GetFiles(_root, "*.bak")); + Assert.Empty(Directory.GetFiles(_root, "*" + ConversionMetadataStore.Suffix)); + } + + [Fact] + public void ATargetThatCannotRepresentTheTextIsStillNotPredicted() + { + // The deliberate limit of this fix, pinned so it is a known gap rather than a + // surprise: the source decodes cleanly, so the preview says it would convert, and + // the real run fails encoding it into a target that has no Cyrillic. + byte[] cyrillic = new UTF8Encoding(false).GetBytes("Здравствуй, мир!\n"); + + Assert.Equal(ConversionRowResult.Converted, Preview(cyrillic, "us-ascii").Result); + + var real = new EntrySink(); + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = "us-ascii", + MaxParallelism = 1, + }, + real.Add, + CancellationToken.None); + + Assert.Equal(ConversionRowResult.Error, real.Single().Result); + Assert.Equal( + nameof(ConversionErrorCode.TargetEncodeError), real.Single().ReasonCode); + } +} diff --git a/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs b/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs new file mode 100644 index 0000000..9bb69ef --- /dev/null +++ b/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs @@ -0,0 +1,143 @@ +namespace EncodingChecker.Tests; + +/// +/// The reason EC gives for a decision must come from the decision. +/// +/// +/// ApplyConversion used to work the reason out again from the four raw facts +/// had already reduced to a +/// . The two copies could not disagree — the expressions +/// were identical and their operands never changed between them — but nothing tied them +/// together, and a refusal reason added to the policy would have fallen through to +/// LegacySourceRequired at the call site: a correct refusal carrying the wrong +/// explanation, with no test able to notice. These tests are what now ties them together. +/// +public sealed class RefusalReasonCodeTests +{ + private static readonly (string Charset, int CodePage)[] Sources = + [ + (ScanEngine.UnknownCharset, 0), + ("windows-1252", 1252), + ("utf-8", 65001), + ("utf-16", 1200), + ]; + + private static readonly (string Charset, int CodePage)[] Targets = + [ + ("utf-16", 1200), + ("utf-8", 65001), + ]; + + /// + /// Every combination of Decide's inputs that ApplyConversion can actually + /// produce, with the decision it reaches. + /// + /// + /// The two flags are mutually exclusive by construction at the call site: a conflict + /// needs an explicit source, and the automatic ambiguity needs the absence of one. + /// + private static IEnumerable<(PlannedAction Action, SourceInterpretation Interpretation, + bool Ambiguous, bool Conflict)> Reachable() + { + foreach ((string sc, int scp) in Sources) + foreach ((string tc, int tcp) in Targets) + foreach (bool sourceHasBom in new[] { false, true }) + foreach (bool targetHasBom in new[] { false, true }) + foreach (bool specified in new[] { false, true }) + foreach (bool unicodeOrAscii in new[] { false, true }) + foreach (bool conflict in new[] { false, true }) + foreach (bool ambiguous in new[] { false, true }) + { + if (conflict && !specified) continue; + if (ambiguous && specified) continue; + + PlannedAction action = ConversionPolicy.Decide( + sc, scp, sourceHasBom, tc, tcp, targetHasBom, + specified, unicodeOrAscii, conflict, ambiguous, + out SourceInterpretation interpretation, out _); + + yield return (action, interpretation, ambiguous, conflict); + } + } + + /// The formula this replaced, kept as the oracle for the change itself. + private static string? PreviousFormula( + PlannedAction action, bool ambiguous, bool conflict) => action switch + { + PlannedAction.Skip => ConversionReasonCodes.UnknownEncoding, + PlannedAction.Refuse when ambiguous => ConversionReasonCodes.AmbiguousBomlessUtf16, + PlannedAction.Refuse => conflict + ? ConversionReasonCodes.ExplicitSourceConflictsWithDetection + : ConversionReasonCodes.LegacySourceRequired, + _ => null, + }; + + [Fact] + public void TheReasonIsUnchangedFromTheFormulaItReplaced() + { + var checkedCombinations = 0; + + foreach ((PlannedAction action, SourceInterpretation interpretation, + bool ambiguous, bool conflict) in Reachable()) + { + checkedCombinations++; + + Assert.Equal( + PreviousFormula(action, ambiguous, conflict), + ConversionPolicy.ReasonCodeFor(action, interpretation)); + } + + // A rule that silently matched nothing would look identical to one that passed. + Assert.Equal(256, checkedCombinations); + } + + [Fact] + public void EveryOutcomeThatLeavesAFileAloneCarriesAReason() + { + // The guard that matters for the next refusal reason someone adds: a decision not + // to convert has to explain itself, and an unmapped interpretation returns null. + foreach ((PlannedAction action, SourceInterpretation interpretation, _, _) in Reachable()) + { + if (action is not (PlannedAction.Refuse or PlannedAction.Skip)) + continue; + + Assert.False( + string.IsNullOrEmpty(ConversionPolicy.ReasonCodeFor(action, interpretation)), + $"{action}/{interpretation} produced no reason code"); + } + } + + [Fact] + public void EachRefusalPathHasItsOwnReason() + { + // Three ways to refuse, three distinct codes: collapsing any two would tell a user + // to do something that cannot resolve their case. + string?[] codes = + [ + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.ExplicitSource), + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.AutomaticUnicodeOrAscii), + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.LegacyNeedsSourceChoice), + ]; + + Assert.Equal(3, codes.Distinct(StringComparer.Ordinal).Count()); + Assert.All(codes, code => Assert.False(string.IsNullOrEmpty(code))); + } + + [Fact] + public void ADecisionToWriteOrLeaveAloneCarriesNoReason() + { + // A reason code on a plain conversion would read as a problem in the report. + // Written as one test rather than a theory because PlannedAction is internal and + // an InlineData parameter would have to be public. + foreach (PlannedAction action in + new[] { PlannedAction.Convert, PlannedAction.Unchanged }) + foreach (SourceInterpretation interpretation in + Enum.GetValues()) + { + Assert.Null(ConversionPolicy.ReasonCodeFor(action, interpretation)); + } + } +} diff --git a/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs b/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs new file mode 100644 index 0000000..8f22628 --- /dev/null +++ b/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs @@ -0,0 +1,100 @@ +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// One file's failure must be one row, not the end of the run. +/// +/// +/// The per-item catch named four exception types. Anything outside that list — a +/// SecurityException from an ACL the enumerator did not surface, a regex timeout, a +/// defect in EC itself — escaped Parallel.ForEach as an AggregateException +/// and took every file the run had not reached with it. The CLI's outer catch names the +/// same four, so it would have surfaced as a crash rather than exit 3. +/// +/// Cancellation and are deliberately still allowed out: +/// one is the user asking to stop, and continuing after the other would be pretending to +/// process rather than processing. +/// +/// +public sealed class ScanFailureIsolationTests +{ + private static ConversionReportEntry Entry(string name) => new() + { + FilePath = @"C:\scan\" + name, + SourceEncoding = "utf-8", + TargetEncoding = "utf-8", + }; + + private static EntrySink Run(Func processItem) + { + var seen = new EntrySink(); + + ScanEngine.RunParallel( + [Entry("a.txt"), Entry("b.txt"), Entry("c.txt")], + maxParallelism: 1, + getPath: entry => entry.FilePath, + processItem: processItem, + onEntry: seen.Add, + CancellationToken.None); + + return seen; + } + + [Theory] + [InlineData(typeof(InvalidOperationException))] + [InlineData(typeof(NullReferenceException))] + [InlineData(typeof(System.Security.SecurityException))] + [InlineData(typeof(System.Text.RegularExpressions.RegexMatchTimeoutException))] + [InlineData(typeof(FormatException))] + public void AnUnlistedFailureBecomesOneRowAndTheRunContinues(Type thrown) + { + EntrySink seen = Run(entry => + entry.FilePath.EndsWith("b.txt", StringComparison.Ordinal) + ? throw (Exception)Activator.CreateInstance(thrown, "boom")! + : entry); + + Assert.Equal(3, seen.Count); + + ConversionReportEntry failed = seen.Single( + e => e.FilePath.EndsWith("b.txt", StringComparison.Ordinal)); + + Assert.Equal(ConversionRowResult.Error, failed.Result); + Assert.Equal(ConversionReasonCodes.ScanFailed, failed.ReasonCode); + Assert.Equal(PlannedAction.Refuse, failed.Action); + Assert.False(failed.ReplacementCommitted); + Assert.Contains("boom", failed.Diagnostic); + + // The other two are untouched, which is the point of isolating the failure. + Assert.All( + seen.Where(e => !e.FilePath.EndsWith("b.txt", StringComparison.Ordinal)), + e => Assert.Null(e.ReasonCode)); + } + + [Theory] + [InlineData(typeof(OperationCanceledException))] + [InlineData(typeof(OutOfMemoryException))] + public void StoppingTheRunIsStillAllowedOut(Type thrown) + { + var seen = new EntrySink(); + + Assert.ThrowsAny(() => + ScanEngine.RunParallel( + [Entry("a.txt")], + maxParallelism: 1, + getPath: entry => entry.FilePath, + processItem: _ => throw (Exception)Activator.CreateInstance(thrown)!, + onEntry: seen.Add, + CancellationToken.None)); + + // Turned into a row, it would read as one file failing while the run carried on. + Assert.Equal(0, seen.Count); + } + + [Fact] + public void AnEntryThatProcessesToNullIsSimplyNotReported() + { + // RefreshSourceSnapshots relies on this, so widening the catch must not change it. + Assert.Equal(0, Run(_ => null).Count); + } +} diff --git a/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs b/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs new file mode 100644 index 0000000..2897dbc --- /dev/null +++ b/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs @@ -0,0 +1,171 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A file already in the target encoding must be left alone however the target was +/// spelled. Codec identity is the code page: "utf-16", "unicode", "ucs-2" and "utf-16le" +/// all name code page 1200, and comparing the labels instead rewrote every such file to +/// identical bytes, discarding its timestamp and creating a backup on the way. +/// +public sealed class TargetAliasIdentityTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-alias-").FullName; + + private readonly string _path; + + public TargetAliasIdentityTests() => _path = Path.Combine(_root, "file.txt"); + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // A leftover temp directory cannot make a passing test wrong. + } + } + + private ConversionReportEntry Convert(string targetLabel) + { + ScanEngine.ParseCharsetLabel(targetLabel, out string charset, out bool writeBom); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = charset, + TargetWriteBom = writeBom, + Backup = true, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Utf16LeWithBom(string text) => + [ + .. new UnicodeEncoding(bigEndian: false, byteOrderMark: true).GetPreamble(), + .. new UnicodeEncoding(bigEndian: false, byteOrderMark: false).GetBytes(text), + ]; + + [Theory] + [InlineData("utf-16-bom")] + [InlineData("unicode-bom")] + [InlineData("ucs-2-bom")] + [InlineData("utf-16le-bom")] + public void EveryAliasOfTheSourceCodecLeavesTheFileAlone(string targetLabel) + { + byte[] original = Utf16LeWithBom("hello alias world\n"); + File.WriteAllBytes(_path, original); + DateTime written = File.GetLastWriteTimeUtc(_path); + + ConversionReportEntry entry = Convert(targetLabel); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Equal(original, File.ReadAllBytes(_path)); + Assert.Equal(written, File.GetLastWriteTimeUtc(_path)); + + // An unchanged file is never read or rewritten, so it earns no recovery artifacts. + Assert.False(File.Exists(_path + ".bak")); + Assert.Empty(Directory.GetFiles(_root, "*" + ConversionMetadataStore.Suffix)); + } + + [Theory] + [InlineData("us-ascii")] + [InlineData("ascii")] + [InlineData("ANSI_X3.4-1968")] + public void EveryAliasOfAsciiLeavesAnAsciiFileAlone(string targetLabel) + { + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + DateTime written = File.GetLastWriteTimeUtc(_path); + + ConversionReportEntry entry = Convert(targetLabel); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(original, File.ReadAllBytes(_path)); + Assert.Equal(written, File.GetLastWriteTimeUtc(_path)); + } + + [Fact] + public void ABomlessUtf16FileIsUnchangedRatherThanRefusedUnderAnAliasTarget() + { + // Spelling the same codec differently used to route this file past the unchanged + // test and into the BOM-less ambiguity refusal, so an alias moved the CLI from + // exit 0 to exit 5 without any file differing. + byte[] original = new UnicodeEncoding(bigEndian: false, byteOrderMark: false) + .GetBytes("a list of ordinary words\n"); + + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-16le"); + + Assert.Equal("utf-16", entry.SourceEncoding); + Assert.False(entry.SourceHasBom); + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Null(entry.ReasonCode); + Assert.Equal(original, File.ReadAllBytes(_path)); + } + + [Fact] + public void AsciiIsStillConvertedToUtf8BecauseTheyAreDifferentCodecs() + { + // Deliberately not folded into the unchanged test. Detection samples only the + // first 64 KiB, so accepting an "us-ascii" label as already being UTF-8 would + // pass silently over a file whose later bytes are neither. + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-8"); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal(ConversionRowResult.Converted, entry.Result); + } + + [Fact] + public void AnUnresolvedCodePageIsNotEvidenceOfSameness() + { + // 0 is the "did not resolve" sentinel plans and journals already use. Two of + // them describe two unknowns, not one shared codec. + Assert.NotEqual( + PlannedAction.Unchanged, + ConversionPolicy.Decide( + "mystery", sourceCodePage: 0, sourceHasBom: false, + "mystery", targetCodePage: 0, targetHasBom: false, + sourceWasSpecified: false, isUnicodeOrAscii: false, + explicitSourceConflictsWithReliableDetection: false, + automaticBomlessUtf16IsAmbiguous: false, + out _, out _)); + } + + [Fact] + public void ADifferingBomPolicyStillConvertsTheSameCodec() + { + // The code page matches, so only the BOM decision separates these. It must still + // be honoured, or "utf-8" and "utf-8-bom" would become the same request. + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-8-bom"); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal( + new UTF8Encoding(encoderShouldEmitUTF8Identifier: true).GetPreamble(), + File.ReadAllBytes(_path).Take(3)); + } +} diff --git a/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs b/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs new file mode 100644 index 0000000..3c930d8 --- /dev/null +++ b/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs @@ -0,0 +1,159 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// "Already in the target encoding" has to be true of the whole file. +/// +/// +/// Detection reads at most 64 KiB, and when the source codec matched the target nothing read +/// further. A file whose first 64 KiB was clean and whose later bytes were not valid in the +/// codec EC had just named was reported Unchanged — and whether EC noticed depended +/// only on which target the caller typed: the same corrupt file came back Error under +/// -Target utf-16 and Unchanged under -Target utf-8. -Validate +/// always read the whole file; the Convert path simply never reached that check once it had +/// decided it had nothing to do. +/// +public sealed class UnchangedVerificationTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-unchanged-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// Clean text well past the detection sample, optionally spoiled at the end. + private static byte[] PastTheSample(bool ascii, bool corrupt) + { + string unit = ascii ? "the quick brown fox jumps over the lazy dog. " : "中文内容测试 "; + var text = new StringBuilder(); + + while (Encoding.UTF8.GetByteCount(text.ToString()) < 70 * 1024) + text.Append(unit); + + byte[] clean = new UTF8Encoding(false).GetBytes(text.ToString()); + + // An overlong "/" — invalid in UTF-8 and outside us-ascii alike. + return corrupt ? [.. clean, 0xC0, 0xAF] : clean; + } + + private ConversionReportEntry Convert(byte[] contents, string target) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = target, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + [Theory] + [InlineData(true, "us-ascii")] + [InlineData(true, "utf-8")] + [InlineData(true, "utf-16")] + [InlineData(false, "us-ascii")] + [InlineData(false, "utf-8")] + [InlineData(false, "utf-16")] + public void TheSameCorruptFileFailsUnderEveryTarget(bool ascii, string target) + { + // The matrix is the finding: the outcome must not depend on which target is named. + byte[] corrupt = PastTheSample(ascii, corrupt: true); + + ConversionReportEntry entry = Convert(corrupt, target); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.False(string.IsNullOrEmpty(entry.ReasonCode)); + Assert.Equal(corrupt, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + } + + [Fact] + public void TheUnchangedPathNamesTheCheckThatFailed() + { + // Reported as the pre-check it is, not as a conversion that went wrong: no write + // was attempted at all. + ConversionReportEntry entry = Convert(PastTheSample(ascii: true, corrupt: true), "us-ascii"); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.False(entry.ReplacementCommitted); + Assert.False(string.IsNullOrEmpty(entry.Diagnostic)); + } + + [Theory] + [InlineData(true, "us-ascii")] + [InlineData(false, "utf-8")] + public void AFileThatReallyIsInTheTargetEncodingIsStillUnchanged(bool ascii, string target) + { + // The check must not start refusing the ordinary case it was added to describe + // honestly — including well past the 64 KiB sample. + byte[] clean = PastTheSample(ascii, corrupt: false); + + ConversionReportEntry entry = Convert(clean, target); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + Assert.Equal(clean, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + } + + [Fact] + public void AShortCleanFileIsStillUnchanged() + { + ConversionReportEntry entry = Convert( + new UTF8Encoding(false).GetBytes("plain ascii content\n"), "us-ascii"); + + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void ConvertAndValidateNowAgreeAboutTheSameFile() + { + // -Validate always caught this. The two modes disagreeing about one file was the + // shape of the defect. + byte[] corrupt = PastTheSample(ascii: true, corrupt: true); + File.WriteAllBytes(Path.Combine(_root, "f.txt"), corrupt); + + var validated = new EntrySink(); + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Validate, + ValidCharsets = ["us-ascii", "utf-8"], + MaxParallelism = 1, + }, + validated.Add, + CancellationToken.None); + + Assert.Equal(ConversionRowResult.Invalid, validated.Single().Result); + Assert.Equal( + ConversionReasonCodes.StrictValidationFailed, validated.Single().ReasonCode); + + // Same file, same reason, from the Convert path. + Assert.Equal( + ConversionReasonCodes.StrictValidationFailed, + Convert(corrupt, "us-ascii").ReasonCode); + } +} diff --git a/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs b/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs new file mode 100644 index 0000000..eb04536 --- /dev/null +++ b/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs @@ -0,0 +1,188 @@ +using System.Diagnostics; +using System.Text; +using System.Text.RegularExpressions; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A folder EC could not read must leave a trace, the way an unreadable file does. +/// +/// +/// An unreadable file becomes a row with ScanFailed and drives exit code 3. An +/// unreadable directory produced a warning on stderr and nothing else: no row, no counter, +/// exit 0 — and nothing whatever in the GUI, which passes no warning callback. Measured +/// against a denied folder: -Validate -FailOnChanges reported "1 file(s) processed" +/// and exited 0, and a scan whose entire base directory was unreadable printed a +/// header-only CSV and exited 0. A run that examined none of the tree could report success, +/// which is the one thing the coverage report exists to prevent. +/// +/// The exit code is deliberately unchanged; docs/CLI.md states that coverage counts +/// do not affect it, and now also states what that means for a script. +/// +/// +public sealed class UnreadableFolderCoverageTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-unreadable-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private static (List Files, DirectoryTraversal.TraversalCounters Counters) + Walk(string baseDirectory) + { + var counters = new DirectoryTraversal.TraversalCounters(); + + List files = + [ + .. DirectoryTraversal.EnumerateFiles( + baseDirectory, + includeSubdirectories: true, + DirectoryTraversal.CompilePatterns(["*"], defaultToMatchAll: true), + [], + excludedFullPaths: null, + onWarning: null, + counters: counters) + ]; + + return (files, counters); + } + + [Fact] + public void AnUnlistableBaseDirectoryIsCounted() + { + // Never created, so listing it fails with DirectoryNotFoundException. This is the + // shape that used to print a header-only CSV and exit 0 with nothing else said. + string ghost = Path.Combine(_root, "never-created-" + Guid.NewGuid().ToString("N")); + + var (files, counters) = Walk(ghost); + + Assert.Empty(files); + Assert.Equal(1, counters.DirectoriesUnreadable); + } + + [Fact] + public void AnUnreadableFolderIsSaidOutLoudInTheCoverageText() + { + // The GUI has no other channel: it passes no warning callback, so this string is + // the whole of what a window user is told. + var counters = new DirectoryTraversal.TraversalCounters(); + counters.CountDirectoryUnreadable(); + + Assert.Equal("1 folder(s) could not be read", MainForm.FormatCoverage(counters)); + } + + [Fact] + public void ItIsCountedApartFromFoldersSkippedOnPurpose() + { + // Three different reasons a folder was not walked, and the report keeps them apart: + // two are policy, one is a failure. + var counters = new DirectoryTraversal.TraversalCounters(); + counters.CountDirectoryExcludedByAttribute(); + counters.CountDirectoryExcludedByName(); + counters.CountDirectoryUnreadable(); + + string coverage = MainForm.FormatCoverage(counters); + + Assert.Contains("1 folder(s) not entered", coverage); + Assert.Contains("1 build/metadata folder(s) not entered", coverage); + Assert.Contains("1 folder(s) could not be read", coverage); + } + + [Fact] + public void AnUnreadableFileStillBecomesARowInstead() + { + // The contrast that made the gap visible: files fail loudly, folders did not. + string path = Path.Combine(_root, "held.txt"); + File.WriteAllText(path, "content", new UTF8Encoding(false)); + + using FileStream exclusive = new(path, FileMode.Open, FileAccess.Read, FileShare.None); + + var entries = new EntrySink(); + var counters = new DirectoryTraversal.TraversalCounters(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Detect, + Counters = counters, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + ConversionReportEntry row = entries.Single(); + + Assert.Equal(ConversionRowResult.Error, row.Result); + Assert.Equal(ConversionReasonCodes.ScanFailed, row.ReasonCode); + Assert.Equal(0, counters.DirectoriesUnreadable); + } + + [Fact] + public void ATreeThatReadsCleanlyReportsNothing() + { + File.WriteAllText(Path.Combine(_root, "a.txt"), "x", new UTF8Encoding(false)); + + var (files, counters) = Walk(_root); + + Assert.Single(files); + Assert.Equal(0, counters.DirectoriesUnreadable); + Assert.Equal(string.Empty, MainForm.FormatCoverage(counters)); + } + + [Fact] + public void ADeniedSubdirectoryIsCountedAndTheScanContinues() + { + string denied = Path.Combine(_root, "denied"); + Directory.CreateDirectory(denied); + File.WriteAllText(Path.Combine(denied, "secret.txt"), "x", new UTF8Encoding(false)); + File.WriteAllText(Path.Combine(_root, "visible.txt"), "x", new UTF8Encoding(false)); + + string user = $"{Environment.UserDomainName}\\{Environment.UserName}"; + + using (Process? deny = Process.Start(new ProcessStartInfo( + "icacls", $"\"{denied}\" /deny \"{user}:(OI)(CI)(RX)\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + })) + { + deny?.WaitForExit(10000); + + if (deny is null || deny.ExitCode != 0) + return; // icacls unavailable here; the ghost-root test still covers the counter. + } + + try + { + var (files, counters) = Walk(_root); + + Assert.Equal("visible.txt", Path.GetFileName(Assert.Single(files))); + Assert.Equal(1, counters.DirectoriesUnreadable); + } + finally + { + using Process? restore = Process.Start(new ProcessStartInfo( + "icacls", $"\"{denied}\" /remove:d \"{user}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }); + + restore?.WaitForExit(10000); + } + } +} diff --git a/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs b/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs new file mode 100644 index 0000000..3121b67 --- /dev/null +++ b/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs @@ -0,0 +1,143 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// Every way -Validate can reject a file has to say which way it was. +/// +/// +/// There are four. Two explained themselves — a file whose whole contents fail strict +/// decoding, and one whose BOM-less byte order cannot be proven. The other two arrived as a +/// bare Invalid with an empty reason column, and they are not the same situation: a +/// charset the caller did not allow means widen the list or convert the file, while an +/// unidentifiable one means EC could not tell what it is. That made -Validate the +/// only outcome in the product whose reason the reader had to reconstruct, from the encoding +/// column and the list they had passed in. +/// +public sealed class ValidateReasonCodeTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-validate-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private ConversionReportEntry Validate(byte[] contents, params string[] allowed) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Validate, + ValidCharsets = allowed, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Cjk(int repeats) => + new UTF8Encoding(false).GetBytes( + string.Concat(Enumerable.Repeat("中文内容测试", repeats))); + + [Fact] + public void AnAllowedCharsetThatDecodesCleanlyPasses() + { + ConversionReportEntry entry = Validate( + new UTF8Encoding(false).GetBytes("plain ascii content\n"), "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void ACharsetOutsideTheAllowedListSaysSo() + { + ConversionReportEntry entry = Validate( + Encoding.GetEncoding("windows-1251").GetBytes("Здравствуй, мир! Это русский текст.\n"), + "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.CharsetNotAllowed, entry.ReasonCode); + Assert.Contains("windows-1251", entry.Diagnostic); + Assert.Contains("not in the allowed list", entry.Diagnostic); + } + + [Fact] + public void AFileEcCannotIdentifyIsNamedAsThatInstead() + { + // The distinction the empty reason column erased: this is not "you did not allow + // it", and -DetectOnly already calls the same condition UnknownEncoding. + ConversionReportEntry entry = Validate([], "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.UnknownEncoding, entry.ReasonCode); + Assert.NotEqual(ConversionReasonCodes.CharsetNotAllowed, entry.ReasonCode); + } + + [Fact] + public void AnAllowedCharsetThatFailsStrictDecodingKeepsItsOwnReason() + { + ConversionReportEntry entry = Validate([.. Cjk(3), 0xE4, 0xB8], "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.Contains("E4", entry.Diagnostic); + } + + [Fact] + public void AnAllowedCharsetWithAnUnprovableByteOrderKeepsItsOwnReason() + { + byte[] utf16 = new UnicodeEncoding(false, false).GetBytes( + string.Concat(Enumerable.Repeat("a quiet line of words\n", 4))); + + ConversionReportEntry entry = Validate(utf16, "utf-16"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.AmbiguousBomlessUtf16, entry.ReasonCode); + } + + [Fact] + public void NoRejectionLeavesTheReaderWithoutAReason() + { + // The property, rather than the four cases: nothing -Validate rejects may arrive + // as a bare Invalid again. + (byte[] Contents, string[] Allowed)[] rejected = + [ + (Encoding.GetEncoding("windows-1251").GetBytes("Здравствуй, мир!\n"), ["utf-8"]), + ([], ["utf-8"]), + ([.. Cjk(3), 0xE4, 0xB8], ["utf-8"]), + (new UnicodeEncoding(false, false).GetBytes( + string.Concat(Enumerable.Repeat("a quiet line of words\n", 4))), ["utf-16"]), + ]; + + foreach ((byte[] contents, string[] allowed) in rejected) + { + ConversionReportEntry entry = Validate(contents, allowed); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.False( + string.IsNullOrEmpty(entry.ReasonCode), + $"a rejected file carried no reason code (encoding was {entry.SourceEncoding})"); + Assert.False( + string.IsNullOrEmpty(entry.Diagnostic), + $"a rejected file carried no diagnostic (encoding was {entry.SourceEncoding})"); + } + } +} diff --git a/sources/EncodingChecker/ConversionPolicy.cs b/sources/EncodingChecker/ConversionPolicy.cs index 763a132..09bd5aa 100644 --- a/sources/EncodingChecker/ConversionPolicy.cs +++ b/sources/EncodingChecker/ConversionPolicy.cs @@ -19,8 +19,14 @@ internal static class ConversionPolicy /// Why, in words a user can act on, when the answer is not a plain conversion. /// /// The character set of the source file. + /// + /// The source codec's canonical code page, or 0 when the label did not resolve. + /// /// Whether the source file has a BOM. /// The character set to convert to. + /// + /// The target codec's canonical code page, or 0 when the label did not resolve. + /// /// Whether to write a BOM when converting to the target charset. /// Whether the source encoding was explicitly specified by the user. /// Whether the source encoding is Unicode or ASCII. @@ -31,8 +37,10 @@ internal static class ConversionPolicy /// The planned action for the file. internal static PlannedAction Decide( string sourceCharset, + int sourceCodePage, bool sourceHasBom, string targetCharset, + int targetCodePage, bool targetHasBom, bool sourceWasSpecified, bool isUnicodeOrAscii, @@ -52,7 +60,14 @@ internal static PlannedAction Decide( } // An unchanged file is not read or rewritten, so no source choice is needed. - if (string.Equals(sourceCharset, targetCharset, StringComparison.OrdinalIgnoreCase) + // + // Codec identity is the code page, not the label. "utf-16", "unicode", "ucs-2" + // and "utf-16le" all name code page 1200, so comparing the strings reported a + // file already in the target as needing conversion, then rewrote it to identical + // bytes - discarding its timestamp, and making -FailOnChanges fail forever. A + // zero code page means the label did not resolve, so it proves nothing. + if (sourceCodePage != 0 + && sourceCodePage == targetCodePage && sourceHasBom == targetHasBom) { sourceInterpretation = SourceInterpretation.NotApplicable; @@ -104,6 +119,38 @@ internal static PlannedAction Decide( _ => ConversionRowResult.Converted, }; + /// + /// The machine-readable reason for a decision, read from the decision itself. + /// + /// + /// Beside because it answers the same question. The caller used to + /// work the reason out again from the raw inputs, re-deriving the distinction + /// had already been handed back to express. The two + /// could not disagree - the expressions were identical and their operands never changed + /// between them - but a refusal reason added to would have fallen + /// through to at the call site: + /// a correct refusal carrying the wrong explanation, with nothing to fail. That already + /// happened once, when the ambiguous BOM-less case had to be bolted on as a guard rather + /// than added as a case. + /// + internal static string? ReasonCodeFor( + PlannedAction action, + SourceInterpretation sourceInterpretation) => (action, sourceInterpretation) switch + { + (PlannedAction.Skip, _) => ConversionReasonCodes.UnknownEncoding, + + (PlannedAction.Refuse, SourceInterpretation.AutomaticUnicodeOrAscii) => + ConversionReasonCodes.AmbiguousBomlessUtf16, + + (PlannedAction.Refuse, SourceInterpretation.ExplicitSource) => + ConversionReasonCodes.ExplicitSourceConflictsWithDetection, + + (PlannedAction.Refuse, SourceInterpretation.LegacyNeedsSourceChoice) => + ConversionReasonCodes.LegacySourceRequired, + + _ => null, + }; + /// /// Whether a refusal can be resolved by the user identifying the original source /// encoding. Kept here so the GUI, plans, and CLI describe the same policy. diff --git a/sources/EncodingChecker/ConversionReport.cs b/sources/EncodingChecker/ConversionReport.cs index 55dc41c..cf69c60 100644 --- a/sources/EncodingChecker/ConversionReport.cs +++ b/sources/EncodingChecker/ConversionReport.cs @@ -225,6 +225,17 @@ internal static class ConversionReasonCodes nameof(ExplicitSourceOnUnprovableBomlessUnicode); internal const string AmbiguousBomlessUtf16 = BomlessUnicodeSafety.AmbiguousReasonCode; internal const string StrictValidationFailed = nameof(StrictValidationFailed); + + /// + /// -Validate identified the file, and its charset is not in the allowed list. + /// + /// + /// Distinct from , which these rows used to be + /// indistinguishable from: one says widen the list or convert the file, the other says + /// EC could not tell what the file is. Both arrived as a bare Invalid with an + /// empty reason, the only outcome in the product that did not explain itself. + /// + internal const string CharsetNotAllowed = nameof(CharsetNotAllowed); internal const string SourceSnapshotFailed = nameof(SourceSnapshotFailed); internal const string BackupFailed = nameof(BackupFailed); internal const string MultipleLeadingByteOrderMarks = nameof(MultipleLeadingByteOrderMarks); diff --git a/sources/EncodingChecker/DirectoryTraversal.cs b/sources/EncodingChecker/DirectoryTraversal.cs index 4f69fcd..5838737 100644 --- a/sources/EncodingChecker/DirectoryTraversal.cs +++ b/sources/EncodingChecker/DirectoryTraversal.cs @@ -57,6 +57,8 @@ internal sealed class TraversalCounters { private int _filesExcludedByAttribute; private int _directoriesExcludedByAttribute; + private int _directoriesExcludedByName; + private int _directoriesUnreadable; private int _filesExcludedAsEcArtifact; /// Matching files skipped for being hidden, system, or reparse points. @@ -66,6 +68,31 @@ internal sealed class TraversalCounters internal int DirectoriesExcludedByAttribute => Volatile.Read(ref _directoriesExcludedByAttribute); + /// + /// Directories not entered because their name is a build or metadata convention. + /// + /// + /// Counted for the same reason the attribute exclusions are. Skipping these is + /// deliberate and documented, but leaving them out of the coverage report let a + /// clean result stand in for complete coverage - the one thing this report exists + /// to prevent - and "build" and "target" are ordinary content directory names + /// outside the conventions they were chosen for. + /// + internal int DirectoriesExcludedByName => + Volatile.Read(ref _directoriesExcludedByName); + + /// Directories EC tried to list and could not. + /// + /// Distinct from the two exclusion counters above, which record directories EC + /// chose not to enter. This one records a failure, and it is the one that used to + /// be invisible: an unreadable file becomes a row and drives exit code 3, while an + /// unreadable directory produced a warning on stderr and nothing else - no row, no + /// count, exit 0. A GUI scan reported nothing at all, because the window passes no + /// warning callback. A run that examined none of the tree could report success. + /// + internal int DirectoriesUnreadable => + Volatile.Read(ref _directoriesUnreadable); + /// /// Matching files skipped for being EC's own backups, sidecars, or temporaries. /// @@ -79,6 +106,12 @@ internal void CountFileExcludedAsEcArtifact() => internal void CountDirectoryExcludedByAttribute() => Interlocked.Increment(ref _directoriesExcludedByAttribute); + + internal void CountDirectoryExcludedByName() => + Interlocked.Increment(ref _directoriesExcludedByName); + + internal void CountDirectoryUnreadable() => + Interlocked.Increment(ref _directoriesUnreadable); } /// @@ -174,6 +207,8 @@ internal static IEnumerable EnumerateFiles( catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException) { + counters?.CountDirectoryUnreadable(); + onWarning?.Invoke( $"Skipping directory (cannot list): {dir}{Environment.NewLine} {ex.Message}"); @@ -235,6 +270,8 @@ internal static IEnumerable EnumerateFiles( catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException) { + counters?.CountDirectoryUnreadable(); + onWarning?.Invoke( $"Skipping directory (cannot list): {dir}{Environment.NewLine} {ex.Message}"); @@ -243,9 +280,11 @@ internal static IEnumerable EnumerateFiles( foreach (DirectoryInfo subdirectory in subdirectories) { - if (ExcludedDirectoryNames.Contains( - subdirectory.Name)) + if (ExcludedDirectoryNames.Contains(subdirectory.Name)) + { + counters?.CountDirectoryExcludedByName(); continue; + } // Do not traverse excluded directories merely to count their contents. // Reporting the directory itself is honest about the unknown scope. diff --git a/sources/EncodingChecker/EncodingConverter.cs b/sources/EncodingChecker/EncodingConverter.cs index c22de0f..3bf85c6 100644 --- a/sources/EncodingChecker/EncodingConverter.cs +++ b/sources/EncodingChecker/EncodingConverter.cs @@ -961,9 +961,13 @@ private static ConversionResult Failure( private static string DescribeDecoderFailure(DecoderFallbackException ex) { - // The index is relative to the failing read chunk. - return - $"invalid byte sequence (offset {ex.Index} within the failing read chunk)."; + // Index is relative to the decoder call that failed, not to the file, and goes + // negative when the sequence began in bytes carried over from the previous call - + // so reporting it as a position produced "offset -2", a place no file has. The + // bytes themselves identify the fault and mean the same thing wherever it happened. + return ex.BytesUnknown is { Length: > 0 } bytes + ? $"invalid byte sequence 0x{System.Convert.ToHexString(bytes)}." + : "invalid byte sequence."; } private static string DescribeEncoderFailure(EncoderFallbackException ex) diff --git a/sources/EncodingChecker/MainForm.Execution.cs b/sources/EncodingChecker/MainForm.Execution.cs index 8ddc48c..3cd0864 100644 --- a/sources/EncodingChecker/MainForm.Execution.cs +++ b/sources/EncodingChecker/MainForm.Execution.cs @@ -372,7 +372,7 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun if (counters is null) return string.Empty; - var parts = new List(3); + var parts = new List(5); if (counters.FilesExcludedByAttribute > 0) { @@ -392,6 +392,20 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun $"{counters.DirectoriesExcludedByAttribute} folder(s) not entered"); } + if (counters.DirectoriesExcludedByName > 0) + { + parts.Add( + $"{counters.DirectoriesExcludedByName} build/metadata folder(s) not entered"); + } + + // The window passes no warning callback, so before this counter a directory the + // scan could not read left no trace in the GUI at all. + if (counters.DirectoriesUnreadable > 0) + { + parts.Add( + $"{counters.DirectoriesUnreadable} folder(s) could not be read"); + } + return string.Join(", ", parts); } diff --git a/sources/EncodingChecker/Program.CliExecution.cs b/sources/EncodingChecker/Program.CliExecution.cs index 691573e..5a18469 100644 --- a/sources/EncodingChecker/Program.CliExecution.cs +++ b/sources/EncodingChecker/Program.CliExecution.cs @@ -381,6 +381,20 @@ .. collectedEntries.OrderBy( + "(hidden, system, or reparse point); their contents were not counted."); } + if (traversalCounters.DirectoriesExcludedByName > 0) + { + Console.Error.WriteLine( + $"{traversalCounters.DirectoriesExcludedByName} folder(s) not entered " + + "(build or metadata name); their contents were not counted."); + } + + if (traversalCounters.DirectoriesUnreadable > 0) + { + Console.Error.WriteLine( + $"{traversalCounters.DirectoriesUnreadable} folder(s) could not be read; " + + "their contents were not examined."); + } + // Quiet mode must not hide the reason for exit code 3. foreach (ConversionReportEntry entry in entries .Where(e => e.Result == ConversionRowResult.Error)) diff --git a/sources/EncodingChecker/Program.cs b/sources/EncodingChecker/Program.cs index eaec018..c2ecb55 100644 --- a/sources/EncodingChecker/Program.cs +++ b/sources/EncodingChecker/Program.cs @@ -67,21 +67,32 @@ private static bool TryAttachToParentConsole() if (!AttachConsole(AttachParentProcess)) return false; - var stdout = new StreamWriter(Console.OpenStandardOutput()) - { - AutoFlush = true - }; - Console.SetOut(stdout); + Console.SetOut( + OpenStandardWriter(Console.OpenStandardOutput(), Console.IsOutputRedirected)); - var stderr = new StreamWriter(Console.OpenStandardError()) - { - AutoFlush = true - }; - Console.SetError(stderr); + Console.SetError( + OpenStandardWriter(Console.OpenStandardError(), Console.IsErrorRedirected)); return true; } + /// + /// A writer encoded for whatever is going to read it. + /// + /// + /// A redirected stream is a file or a pipe, so UTF-8 is the useful answer and matches + /// the -Report file apart from its BOM. A console decodes with its own code page + /// instead, and these writers were UTF-8 either way: handing CP437 the UTF-8 for + /// "Grüße" displays "Gr├╝├ƒe", which is precisely the failure this program + /// exists to find. Characters the console cannot represent become "?", which is visibly + /// lossy rather than quietly wrong. + /// + internal static StreamWriter OpenStandardWriter(Stream stream, bool redirected) => + new(stream, redirected ? new UTF8Encoding(false) : Console.OutputEncoding) + { + AutoFlush = true, + }; + #endregion #region Console mode @@ -198,7 +209,7 @@ A pattern with a separator matches relative paths. -Quiet Suppress per-file CSV and normal summaries. Errors and coverage warnings still go to stderr. -Verbose Include error details and a result breakdown. - -MaxParallelism Maximum simultaneous files; default is min(CPU count, 4). + -MaxParallelism Maximum simultaneous files; default is min(CPU count, 8). -FailOnChanges Return exit code 2 if files need conversion (or fail validation). Useful for CI. diff --git a/sources/EncodingChecker/Properties/AssemblyInfo.cs b/sources/EncodingChecker/Properties/AssemblyInfo.cs index 26c7f1a..a36b31e 100644 --- a/sources/EncodingChecker/Properties/AssemblyInfo.cs +++ b/sources/EncodingChecker/Properties/AssemblyInfo.cs @@ -41,5 +41,5 @@ // You can specify all the values, or you can default the Build and Revision Numbers // by using the '*' as shown below: // [assembly: AssemblyVersion("1.0.*")] -[assembly: AssemblyVersion("3.11.2.0")] -[assembly: AssemblyFileVersion("3.11.2.0")] +[assembly: AssemblyVersion("3.12.0.0")] +[assembly: AssemblyFileVersion("3.12.0.0")] diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index ded41e7..712e252 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -100,8 +100,12 @@ internal static class ScanEngine // above the point where CPU count would matter. Measured on 2,000 files: 4 gave // 4,207 ms and 8 gave 2,511 ms without backups, 10,516 and 7,305 with them. Past 8 // the curve flattens, and backup runs stop improving entirely. + // Named rather than inlined so the documentation stating it can be asserted against + // it. Both the help text and CLI.md still said 4 a release after this became 8. + internal const int MaxParallelismCap = 8; + internal static readonly int DefaultMaxParallelism = - Math.Min(Environment.ProcessorCount, 8); + Math.Min(Environment.ProcessorCount, MaxParallelismCap); /// Charset label used when the source encoding cannot be established. internal const string UnknownCharset = "(Unknown)"; @@ -552,13 +556,29 @@ options.ValidCharsets is not null && ? ConversionRowResult.Unchanged : ConversionRowResult.Invalid; - if (isValid && entry.Result == ConversionRowResult.Invalid) + if (!isValid) + { + // Two unlike situations used to arrive here as the same bare Invalid: + // a file EC could not identify, and one it identified as something the + // caller did not allow. Every other mode names both. Leaving these + // blank made -Validate the only outcome whose reason the reader had to + // reconstruct from the encoding column and the list they passed in. + bool identified = sourceCharset != UnknownCharset; + + entry.ReasonCode = identified + ? ConversionReasonCodes.CharsetNotAllowed + : ConversionReasonCodes.UnknownEncoding; + + entry.Diagnostic = identified + ? $"The file is {label}, which is not in the allowed list." + : "The file's encoding could not be identified from its contents."; + } + else if (entry.Result == ConversionRowResult.Invalid) { entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; entry.Diagnostic = validationDiagnostic; } - else if (entry.Result == ConversionRowResult.Unchanged && - entry.HasAmbiguousBomlessUtf16) + else if (entry.HasAmbiguousBomlessUtf16) { // The two byte orders are separate entries in the allowed set; the // label matched only because .NET names both "utf-16". Passing the @@ -823,8 +843,10 @@ private static void ApplyConversion( PlannedAction action = ConversionPolicy.Decide( sourceCharset, + sourceEncoding.CodePage, sourceHasBom, targetCharset, + targetEncoding.CodePage, targetWriteBom, entry.SourceEncodingWasSpecified, TextEncoding.IsUnicodeOrAscii(sourceEncoding), @@ -851,18 +873,7 @@ approved.Action is not PlannedAction.Convert && entry.Action = action; entry.SourceInterpretation = sourceInterpretation; - entry.ReasonCode = action switch - { - PlannedAction.Skip => ConversionReasonCodes.UnknownEncoding, - PlannedAction.Refuse when automaticBomlessUtf16IsAmbiguous => - ConversionReasonCodes.AmbiguousBomlessUtf16, - PlannedAction.Refuse => entry.SourceEncodingWasSpecified && - entry.HasReliableUnicodeDetection && automaticallyDetected is not null && - automaticallyDetected.CodePage != sourceEncoding.CodePage - ? ConversionReasonCodes.ExplicitSourceConflictsWithDetection - : ConversionReasonCodes.LegacySourceRequired, - _ => null, - }; + entry.ReasonCode = ConversionPolicy.ReasonCodeFor(action, sourceInterpretation); // A retry must not carry a diagnostic from an earlier failed attempt. // The optional BOM-less Unicode advisory below is added back for this pass. @@ -894,6 +905,26 @@ automaticallyDetected is not null && + "your explicit selection and kept all strict conversion checks enabled."; } + // "Already in the target encoding" is a claim about the whole file, and detection + // saw at most the first 64 KiB of it. Without this check a file whose later bytes + // are not valid in the codec just named was reported as already correct, and + // whether EC noticed depended only on which target the caller happened to type: + // the same corrupt file was an Error under -Target utf-16 and Unchanged under + // -Target utf-8. -Validate has always read the whole file; this is the same check, + // reached from the one path that had decided it had nothing to do. + if (action == PlannedAction.Unchanged && + !StrictFileValidation.TryValidateFile( + path, sourceEncoding, out string? unchangedDiagnostic)) + { + entry.Result = ConversionRowResult.Error; + entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; + entry.Diagnostic = unchangedDiagnostic; + + // Nothing was written, and nothing was going to be. + entry.ReplacementCommitted = false; + return; + } + if (action != PlannedAction.Convert) { entry.Result = ConversionPolicy.ToRowResult(action); @@ -916,6 +947,27 @@ automaticallyDetected is not null && if (whatIf) { + // A preview saying "would be converted" has to have read what it is promising + // about. Nothing here decoded the file, so -Plan - which sets WhatIf - recorded + // Action=Convert with no reason for a source that cannot be read, exited 0, and + // showed the reviewer nothing; the failure surfaced at -Apply, after approval + // and part-way through the batch. Refuse rather than schedule it, so the plan + // never carries a file it cannot carry out. + // + // The decode only. A source that reads cleanly can still fail on a target that + // cannot represent it, and no amount of reading the source predicts that; + // closing that half means running the whole conversion into a discarded buffer. + if (!StrictFileValidation.TryValidateFile( + path, sourceEncoding, out string? previewDiagnostic)) + { + entry.Action = PlannedAction.Refuse; + entry.Result = ConversionRowResult.Error; + entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; + entry.Diagnostic = previewDiagnostic; + entry.ReplacementCommitted = false; + return; + } + entry.Result = ConversionRowResult.Converted; // "would be converted" return; } @@ -1216,7 +1268,13 @@ private static void CreateBackup(string path) /// Processes items with bounded parallelism and isolates per-file errors. /// Cancellation propagates normally. /// - private static void RunParallel( + /// + /// Internal rather than private so the isolation itself can be tested. No file can be + /// made to throw the exceptions this has to survive - that is what makes them the + /// dangerous ones - so the only way to prove one file's failure stays one row is to + /// hand it a that throws. + /// + internal static void RunParallel( IEnumerable items, int maxParallelism, Func getPath, @@ -1244,11 +1302,15 @@ private static void RunParallel( { entry = processItem(item); } + // One file's failure is one row. This used to name four exception types, + // so anything else - a SecurityException the enumerator did not surface, a + // regex timeout, a defect in this code - escaped Parallel.ForEach as an + // AggregateException and took every file the run had not reached yet with + // it. Cancellation still propagates, and OutOfMemoryException is left alone + // because carrying on after it would be pretending to process, not + // processing. catch (Exception ex) when ( - ex is IOException or - UnauthorizedAccessException or - ArgumentException or - NotSupportedException) + ex is not OperationCanceledException and not OutOfMemoryException) { if (item is ConversionReportEntry existing) {