diff --git a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs index 3d760f86..58e11db9 100644 --- a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs +++ b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs @@ -80,6 +80,48 @@ public async Task AcceptSingle_NotEmpty() await Assert.That(tracker.TrackingAny).IsTrue(); } + /// + /// A delete that cannot be done. The menu and hot key paths called File.Delete straight, so a + /// read-only or open verified file threw out of the click handler - onto the UI thread, where + /// nothing hooks Application.ThreadException - and the entry had already been untracked, so + /// the pending delete went with it. + /// + [Test] + public async Task AcceptLeavesAnUndeletableFileTracked() + { + await using var tracker = new RecordingTracker(); + var tracked = tracker.AddDelete(file1); + + using (File.Open(file1, FileMode.Open, FileAccess.Read, FileShare.None)) + { + tracker.Accept(tracked); + } + + await Assert.That(tracker.Deletes).HasSingleItem(); + await Assert.That(File.Exists(file1)).IsTrue(); + } + + /// + /// And one bad delete does not stop the rest of Accept all. Unguarded, the throw skipped the + /// remaining deletes, every pending move, and the snapshots. + /// + [Test] + public async Task AcceptAllContinuesPastAnUndeletableFile() + { + await using var tracker = new RecordingTracker(); + tracker.AddDelete(file1); + tracker.AddDelete(file2); + + using (File.Open(file1, FileMode.Open, FileAccess.Read, FileShare.None)) + { + await tracker.AcceptAll(); + } + + // The one that could go, went; the one that could not is still pending + await Assert.That(File.Exists(file2)).IsFalse(); + await Assert.That(tracker.Deletes).HasSingleItem(); + } + public void Dispose() { File.Delete(file1); diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 5ab19434..089e3602 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -404,22 +404,30 @@ public TrackedDelete AddDelete(string file) => return existing; }); + /// + /// Through , which is what the wire path has always + /// used: it catches, re-tracks so the delete can be retried, and reports why. + /// + /// These called File.Delete straight, so a read-only or open verified file threw out of a menu + /// click or a hot key - onto the UI thread, where nothing hooks Application.ThreadException - + /// and the entry was already untracked by then, so the pending delete was lost with it. + /// + /// public void Accept(TrackedDelete delete) { - if (deletes.TryRemove(delete.File, out var removed)) + var (ok, message) = AcceptTracked(delete); + if (!ok && + message != null) { - File.Delete(removed.File); + Log.Error(message); } } public void Accept(IEnumerable toAccept) { - foreach (var delete in toAccept) + foreach (var delete in toAccept.ToList()) { - if (deletes.TryRemove(delete.File, out var removed)) - { - File.Delete(removed.File); - } + Accept(delete); } } @@ -706,12 +714,13 @@ public Task AcceptAll() void AcceptAllDeletes() { - foreach (var delete in deletes.Values) + // One at a time, and no Clear afterwards: a delete that fails re-tracks itself, and + // clearing would throw that away. Unguarded, the first bad one also took AcceptMoves and + // AcceptAllSnapshots with it, so "Accept all" stopped at the first read-only file + foreach (var delete in deletes.Values.ToList()) { - File.Delete(delete.File); + Accept(delete); } - - deletes.Clear(); } public ICollection Deletes => deletes.Values;