diff --git a/FileDeduplicator.Test/DeduplicatorTests.cs b/FileDeduplicator.Test/DeduplicatorTests.cs index 89b7a34..dea0d86 100644 --- a/FileDeduplicator.Test/DeduplicatorTests.cs +++ b/FileDeduplicator.Test/DeduplicatorTests.cs @@ -232,6 +232,149 @@ 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."); + } + + /// + /// 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. + /// + [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 +388,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..3975caa 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -41,15 +41,30 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList errors = []; + List skipped = []; foreach (DuplicateGroup group in duplicateGroups) { AbsoluteFilePath keeper = SelectFileToKeep(group.Files); - foreach (AbsoluteFilePath file in 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)) { - if (file == keeper) + SkipWholeGroup(group, keeper, keeperReason, skipped); + continue; + } + + foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper)) + { + // 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. + if (!StillMatchesGroup(file, group.Hash, out string? reason)) { + Skip(file, reason, skipped); continue; } @@ -70,7 +85,63 @@ 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. + /// + /// 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 +152,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.");