From df718a3d0880a5f8e12316990c63ac18c2fb776b Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 23:24:58 +0000 Subject: [PATCH 1/3] fix: re-verify each file's hash before deleting it [minor] Deduplicator.DeleteDuplicates drove deletion entirely from the hash map computed before the "Proceed with deletion? (y/N)" prompt, an interactive pause of unbounded length. A file that changed during that pause -- a sync client, an editor autosave, a restored backup -- was still deleted as a duplicate, permanently losing content that was no longer a duplicate at the moment of deletion. Each file is now re-hashed immediately before it is deleted and skipped if it no longer matches its group's hash. The kept copy is verified first: if it changed, the whole group is left alone, since deleting the rest would destroy the only remaining copies of the grouped content. Skips are reported rather than swallowed, through the new DeduplicationResult.SkippedFiles and the Deduplicate summary, so a preserved file is visible instead of silently counted as untouched. Fixes #112 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011ghFwAUJnfH7mfoWm1bCCh --- FileDeduplicator.Test/DeduplicatorTests.cs | 115 +++++++++++++++++++++ FileDeduplicator/Deduplicator.cs | 83 ++++++++++++++- FileDeduplicator/Verbs/Deduplicate.cs | 10 ++ 3 files changed, 206 insertions(+), 2 deletions(-) diff --git a/FileDeduplicator.Test/DeduplicatorTests.cs b/FileDeduplicator.Test/DeduplicatorTests.cs index 89b7a34..442b934 100644 --- a/FileDeduplicator.Test/DeduplicatorTests.cs +++ b/FileDeduplicator.Test/DeduplicatorTests.cs @@ -232,6 +232,120 @@ public void ReclaimedBytesCountsOnlyTheDeletedCopies() Assert.AreEqual(200, result.BytesReclaimed); } + /// + /// Grouping happens before an interactive confirmation prompt of unbounded length, so a file + /// can stop being a duplicate before the delete pass runs. Such a file must be preserved and + /// reported, never deleted on the strength of the stale hash. + /// + [TestMethod] + public void AFileThatChangedAfterGroupingIsPreservedAndReported() + { + // Arrange -- three copies; a.txt is the keeper, bb.txt and ccc.txt are up for deletion + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath changed = tree.Write("bb.txt", "shared"); + AbsoluteFilePath stillDuplicate = tree.Write("ccc.txt", "shared"); + IReadOnlyList duplicates = Duplicates(FileHasher.HashFiles([keeper, changed, stillDuplicate])); + + // Arrange -- bb.txt is rewritten during the confirmation pause, as a sync client or an + // editor autosave would do + _ = tree.Write("bb.txt", "no longer the same content"); + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert + Assert.IsTrue(TempTree.Exists(changed), "A file that is no longer a duplicate must not be deleted."); + Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive."); + Assert.IsFalse(TempTree.Exists(stillDuplicate), "A copy that is still identical should still be deleted."); + Assert.AreEqual(1, result.DeletedCount); + Assert.ContainsSingle(result.SkippedFiles); + Assert.AreEqual(changed, result.SkippedFiles[0].Path); + Assert.Contains("changed", result.SkippedFiles[0].Reason); + Assert.IsEmpty(result.Errors); + } + + /// + /// If the copy being kept changed during the pause, deleting the rest would destroy the only + /// remaining copies of the grouped content, so the whole group must be left alone. + /// + [TestMethod] + public void AChangedKeeperPreservesTheWholeGroup() + { + // Arrange + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath copyOne = tree.Write("bb.txt", "shared"); + AbsoluteFilePath copyTwo = tree.Write("ccc.txt", "shared"); + IReadOnlyList duplicates = Duplicates(FileHasher.HashFiles([keeper, copyOne, copyTwo])); + + // Arrange -- the keeper is rewritten during the confirmation pause + _ = tree.Write("a.txt", "the keeper was overwritten"); + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert + Assert.AreEqual(0, result.DeletedCount); + Assert.AreEqual(0, result.BytesReclaimed); + Assert.HasCount(2, result.SkippedFiles); + Assert.IsTrue(TempTree.Exists(copyOne), "The grouped content must survive somewhere."); + Assert.IsTrue(TempTree.Exists(copyTwo), "The grouped content must survive somewhere."); + } + + /// + /// A file that disappears before the delete pass must be reported as skipped rather than + /// counted among the deletions, so the reclaimed total stays honest. + /// + [TestMethod] + public void AFileThatVanishedBeforeDeletionIsReportedNotCounted() + { + // Arrange + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath vanishing = tree.Write("bb.txt", "shared"); + IReadOnlyList duplicates = Duplicates(FileHasher.HashFiles([keeper, vanishing])); + + // Arrange -- something else removes it during the confirmation pause + File.Delete(vanishing.WeakString); + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert + Assert.AreEqual(0, result.DeletedCount); + Assert.AreEqual(0, result.BytesReclaimed); + Assert.ContainsSingle(result.SkippedFiles); + Assert.AreEqual(vanishing, result.SkippedFiles[0].Path); + Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive."); + } + + /// + /// The re-verification must not degrade into skipping everything: untouched duplicates are + /// still deleted, and nothing is reported as preserved. + /// + [TestMethod] + public void UntouchedDuplicatesAreDeletedWithNothingSkipped() + { + // Arrange + using TempTree tree = new(); + Dictionary hashes = FileHasher.HashFiles( + [ + tree.Write("a.txt", "shared"), + tree.Write("bb.txt", "shared"), + tree.Write("ccc.txt", "shared"), + ]); + IReadOnlyList duplicates = Duplicates(hashes); + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert + Assert.AreEqual(2, result.DeletedCount); + Assert.IsEmpty(result.SkippedFiles); + Assert.IsEmpty(result.Errors); + } + /// /// Deleting nothing must report nothing, rather than throwing on an empty group list. /// @@ -245,5 +359,6 @@ public void DeletingAnEmptyGroupListIsANoOp() Assert.AreEqual(0, result.DeletedCount); Assert.AreEqual(0, result.BytesReclaimed); Assert.IsEmpty(result.Errors); + Assert.IsEmpty(result.SkippedFiles); } } diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index ea3164e..f37b76c 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -41,11 +41,26 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList errors = []; + List skipped = []; foreach (DuplicateGroup group in duplicateGroups) { AbsoluteFilePath keeper = SelectFileToKeep(group.Files); + // The grouping was computed before the confirmation prompt, which is an interactive + // pause of unbounded length. If the copy being kept no longer holds the group's + // content, deleting the others would destroy the only remaining copies of it, so the + // whole group is left alone. + if (!StillMatchesGroup(keeper, group.Hash, out string? keeperReason)) + { + foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper)) + { + Skip(file, $"the copy being kept ({keeper}) {keeperReason}", skipped); + } + + continue; + } + foreach (AbsoluteFilePath file in group.Files) { if (file == keeper) @@ -53,6 +68,15 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList + /// Re-reads a file and reports whether it still holds the content its duplicate group was + /// formed from. + /// + /// The file to re-hash. + /// The hash the group was formed from. + /// When the answer is no, why -- phrased to follow the file's name. + /// if the file still hashes to . + private static bool StillMatchesGroup(AbsoluteFilePath file, string groupHash, out string? reason) + { + try + { + if (string.Equals(FileHasher.ComputeHash(file), groupHash, StringComparison.Ordinal)) + { + reason = null; + return true; + } + + reason = "changed since it was scanned, so it is no longer a duplicate"; + return false; + } + catch (IOException ex) + { + reason = $"could not be re-read to confirm it is still a duplicate: {ex.Message}"; + return false; + } + catch (UnauthorizedAccessException ex) + { + reason = $"could not be re-read to confirm it is still a duplicate: {ex.Message}"; + return false; + } + } + + private static void Skip(AbsoluteFilePath file, string? reason, List skipped) + { + SkippedFile skip = new(file, reason ?? "could not be confirmed as a duplicate"); + skipped.Add(skip); + Console.WriteLine($" Skipped: {file} -- {skip.Reason}"); } } @@ -81,9 +145,24 @@ internal sealed class DuplicateGroup(string hash, List files) internal long FileSize { get; } = new FileInfo(files[0].WeakString).Length; } -internal sealed class DeduplicationResult(int deletedCount, long bytesReclaimed, List errors) +internal sealed class DeduplicationResult(int deletedCount, long bytesReclaimed, List errors, List skippedFiles) { internal int DeletedCount { get; } = deletedCount; internal long BytesReclaimed { get; } = bytesReclaimed; internal IReadOnlyList Errors { get; } = errors; + + /// + /// Gets the files that were proposed for deletion but left on disk because they could no + /// longer be confirmed as duplicates. + /// + internal IReadOnlyList SkippedFiles { get; } = skippedFiles; +} + +/// +/// A file that was preserved instead of deleted, and why. +/// +internal sealed class SkippedFile(AbsoluteFilePath path, string reason) +{ + internal AbsoluteFilePath Path { get; } = path; + internal string Reason { get; } = reason; } diff --git a/FileDeduplicator/Verbs/Deduplicate.cs b/FileDeduplicator/Verbs/Deduplicate.cs index 5910c43..de3e770 100644 --- a/FileDeduplicator/Verbs/Deduplicate.cs +++ b/FileDeduplicator/Verbs/Deduplicate.cs @@ -83,6 +83,16 @@ internal override void Run(Deduplicate options) Console.WriteLine($"Deleted {result.DeletedCount} file(s)."); Console.WriteLine($"Reclaimed {FormatBytes(result.BytesReclaimed)} of disk space."); + if (result.SkippedFiles.Count > 0) + { + Console.WriteLine($"Preserved {result.SkippedFiles.Count} file(s) that could no longer be confirmed as duplicates:"); + + foreach (SkippedFile skipped in result.SkippedFiles) + { + Console.WriteLine($" {skipped.Path} -- {skipped.Reason}"); + } + } + if (result.Errors.Count > 0) { Console.WriteLine($"Encountered {result.Errors.Count} error(s) during deletion."); From b1ae8f05b3331230c89fffce20afa8da694afd55 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 23:35:53 +0000 Subject: [PATCH 2/3] test: cover the unreadable-path branch of the pre-delete verification [patch] SonarCloud's quality gate failed on coverage of new code (78.1%, gate 80%). The uncovered lines were the UnauthorizedAccessException arm of StillMatchesGroup and the verb's summary block. Adds the case that exercises the first: the file at a grouped path is replaced by a directory of the same name during the confirmation pause, so re-reading it raises UnauthorizedAccessException. It is skipped rather than deleted, which also keeps File.Delete from raising the same exception uncaught out of the delete path. New-code coverage measured locally from the cobertura report: 28/32 = 87.5%. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011ghFwAUJnfH7mfoWm1bCCh --- FileDeduplicator.Test/DeduplicatorTests.cs | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/FileDeduplicator.Test/DeduplicatorTests.cs b/FileDeduplicator.Test/DeduplicatorTests.cs index 442b934..dea0d86 100644 --- a/FileDeduplicator.Test/DeduplicatorTests.cs +++ b/FileDeduplicator.Test/DeduplicatorTests.cs @@ -320,6 +320,35 @@ public void AFileThatVanishedBeforeDeletionIsReportedNotCounted() Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive."); } + /// + /// A path whose file was replaced by a directory of the same name cannot be re-read, so it + /// must be skipped. Attempting the delete instead would raise + /// , which the delete path does not catch. + /// + [TestMethod] + public void APathReplacedByADirectoryIsSkippedRatherThanDeleted() + { + // Arrange + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath replaced = tree.Write("bb.txt", "shared"); + IReadOnlyList duplicates = Duplicates(FileHasher.HashFiles([keeper, replaced])); + + // Arrange -- during the confirmation pause the file becomes a directory of the same name + File.Delete(replaced.WeakString); + _ = Directory.CreateDirectory(replaced.WeakString); + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert + Assert.AreEqual(0, result.DeletedCount); + Assert.ContainsSingle(result.SkippedFiles); + Assert.AreEqual(replaced, result.SkippedFiles[0].Path); + Assert.IsTrue(Directory.Exists(replaced.WeakString), "The directory now at that path must be left alone."); + Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive."); + } + /// /// The re-verification must not degrade into skipping everything: untouched duplicates are /// still deleted, and nothing is reported as preserved. From 221c593777b7d30a9594fe096de08f6de6775b94 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 23:46:46 +0000 Subject: [PATCH 3/3] refactor: lift the keeper-mismatch path out of DeleteDuplicates [patch] SonarCloud reported S3776 on DeleteDuplicates: cognitive complexity 17 against the 15 allowed, from the verification branches added for #112. Moves the preserve-the-whole-group loop into SkipWholeGroup, and filters the keeper out of the deletion loop with Where rather than a continue inside the body. Behaviour is unchanged; the method scores 11. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011ghFwAUJnfH7mfoWm1bCCh --- FileDeduplicator/Deduplicator.cs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index f37b76c..3975caa 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -53,21 +53,12 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList f != keeper)) - { - Skip(file, $"the copy being kept ({keeper}) {keeperReason}", skipped); - } - + SkipWholeGroup(group, keeper, keeperReason, skipped); continue; } - foreach (AbsoluteFilePath file in group.Files) + foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper)) { - if (file == keeper) - { - continue; - } - // Re-hash immediately before deleting: a file that changed since the scan is no // longer a duplicate, and deleting it would be irreversible loss of content that // exists nowhere else. @@ -97,6 +88,22 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList + /// Preserves every copy in a group, because the copy that would have been kept no longer + /// holds the group's content. + /// + /// The group to leave on disk. + /// The copy that would have been kept. + /// What the keeper did, phrased to follow its name. + /// The list to record the preserved files on. + private static void SkipWholeGroup(DuplicateGroup group, AbsoluteFilePath keeper, string? keeperReason, List skipped) + { + foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper)) + { + Skip(file, $"the copy being kept ({keeper}) {keeperReason}", skipped); + } + } + /// /// Re-reads a file and reports whether it still holds the content its duplicate group was /// formed from.