diff options
| -rw-r--r-- | cmd/krino/undo.go | 2 | ||||
| -rw-r--r-- | internal/engine/apply.go | 61 | ||||
| -rw-r--r-- | internal/engine/apply_test.go | 79 | ||||
| -rw-r--r-- | internal/journal/read.go | 19 | ||||
| -rw-r--r-- | internal/journal/read_test.go | 49 |
5 files changed, 207 insertions, 3 deletions
diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go index 2d170a4..d21c40e 100644 --- a/cmd/krino/undo.go +++ b/cmd/krino/undo.go @@ -513,7 +513,7 @@ type undoRow struct { func undoRows(files []engine.UndoFile) []undoRow { var rows []undoRow for i, f := range files { - label := display(f.Dir + "/" + f.File) + label := display(f.Label()) if f.Refused != "" { rows = append(rows, undoRow{num: strconv.Itoa(i + 1), file: label, action: "refused: " + display(f.Refused)}) continue diff --git a/internal/engine/apply.go b/internal/engine/apply.go index e2d4db8..f54169c 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -344,6 +344,18 @@ type UndoFile struct { Declined bool } +// Label names the file for display: the directory it belongs to and the +// path within it, except for a file that lies outside any sorted directory +// - one displaced into the Trash by a chain that then deleted itself - whose +// path is absolute and stands alone. Dir stays set on those either way: it +// is what the undo locks. +func (uf UndoFile) Label() string { + if filepath.IsAbs(uf.File) { + return xdg.Abbrev(uf.File) + } + return uf.Dir + "/" + uf.File +} + // UndoStep is the reversal of one logged step. type UndoStep struct { Original journal.Entry // the step being reversed @@ -420,6 +432,13 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { if len(uf.Steps) == 0 && uf.Refused == "" { continue } + // A file refused outright still displaced someone else's file into + // the Trash, and that file is a different file - §7.4 promises undo + // restores it. Offered on its own, since the refusal is about the + // file that cannot come back, not about this one. + if rest, ok := displacedUndoFile(k.dir, byFile[k], reversed); ok { + up.Files = append(up.Files, rest) + } if uf.Refused == "" && onlyOccupiedDirectoryRemovals(uf.Steps) { // Nothing of the file itself is left to reverse, only directories // the run made that something else still occupies: offering them @@ -585,6 +604,48 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R return uf } +// displacedUndoFile offers the file a refused chain trashed to make room. +// It applies only to a chain planUndoFile refuses as a whole - a reversible +// chain reverses its own displace as one of its steps - and only to a +// refusal the displaced file is not itself the cause of: a permanent +// delete, which ends the deleted file for good while leaving the file it +// displaced sitting in the Trash, recoverable, with its original path now +// free. +// +// It is keyed by the displaced path rather than the chain's file, because +// that is what it puts back; a plan that named the chain's file twice would +// read as one file being reversed in two places. +func displacedUndoFile(dir string, ents []journal.Entry, reversed map[journal.ReversedKey]int) (UndoFile, bool) { + deleted := false + for _, en := range ents { + if en.Status == "ok" && en.Action == "delete" { + deleted = true + } + } + if !deleted { + return UndoFile{}, false + } + var out UndoFile + for i := len(ents) - 1; i >= 0; i-- { + en := ents[i] + if en.Status != "ok" || en.Action != "displace" { + continue + } + step := reverseStep(en) + if k := (journal.ReversedKey{Dir: dir, File: en.File, Action: step.Action, Src: step.Src}); reversed[k] > 0 { + reversed[k]-- + continue + } + out.Dir = dir + out.File = en.Src + out.Steps = append(out.Steps, step) + if step.Refused != "" && out.Refused == "" { + out.Refused = step.Refused + } + } + return out, len(out.Steps) > 0 +} + // undoProjection tracks what the reversal steps planUndoFile has already // queued (in the order they will execute) will do to the filesystem, so a // later step's occupancy check can tell a real, external occupant from a diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index a6b3185..c832e3a 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -1463,3 +1463,82 @@ func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *tes t.Errorf("the undo run %q itself must never read as Undone", undoRun) } } + +// TestPermanentDeleteStillRestoresWhatItDisplaced: a chain that overwrites +// and then permanently deletes trashes a file the user owned to make room. +// That file is a different file, and §7.4 promises of it: "move the +// existing target to Trash first (logged, so undo restores it)". Walking +// the file's entries used to stop dead at the permanent delete, so the +// displace was never reached and the user's file stayed in the Trash with +// krino reporting the run fully undone. +func TestPermanentDeleteStillRestoresWhatItDisplaced(t *testing.T) { + // A real trash entry, so the reversal is offered rather than refused + // for a reason that has nothing to do with this test. + h := sandbox(t) + entry := filepath.Join(trash.Dir(), "files", "a.pdf") + if err := os.MkdirAll(filepath.Dir(entry), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(entry, []byte("the file that was in the way"), 0o644); err != nil { + t.Fatal(err) + } + fi, err := os.Lstat(entry) + if err != nil { + t.Fatal(err) + } + displaced := filepath.Join(h, "archive", "a.pdf") + info := filepath.Join(trash.Dir(), "info", "a.pdf.trashinfo") + if err := os.MkdirAll(filepath.Dir(info), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(info, []byte("[Trash Info]\nPath="+displaced+"\nDeletionDate=2026-09-17T00:00:00\n"), 0o644); err != nil { + t.Fatal(err) + } + ents := []journal.Entry{ + // Chronological, as the log has them: the displace first, then the + // move that needed the name, then the permanent delete. + {Action: "displace", Status: "ok", File: "a.pdf", Dir: "dl", Step: 1, + Src: displaced, Dst: entry, Detail: "a.pdf", + Size: fi.Size(), ModTime: fi.ModTime()}, + {Action: "move", Status: "ok", File: "a.pdf", Dir: "dl", Step: 1, + Src: filepath.Join(h, "dl", "a.pdf"), Dst: displaced}, + {Action: "delete", Status: "ok", File: "a.pdf", Dir: "dl", Step: 2, + Src: displaced}, + } + uf := planUndoFile("dl", "a.pdf", ents, map[journal.ReversedKey]int{}) + if uf.Refused == "" { + t.Error("the permanently deleted file is no longer refused") + } + + rest, ok := displacedUndoFile("dl", ents, map[journal.ReversedKey]int{}) + if !ok { + t.Fatal("the displaced file was not offered for reversal at all") + } + if rest.Refused != "" { + t.Errorf("the displaced file is refused: %q", rest.Refused) + } + if len(rest.Steps) != 1 || rest.Steps[0].Action != "undo-displace" { + t.Fatalf("steps = %+v; want one undo-displace", rest.Steps) + } + if rest.Steps[0].Dst != displaced { + t.Errorf("the reversal puts the file at %q, want %q", rest.Steps[0].Dst, displaced) + } + if rest.File != displaced { + t.Errorf("the offered file is %q, want the displaced file %q", rest.File, displaced) + } +} + +// TestDisplacedFileIsNotOfferedTwice: when the chain's own file is +// reversible, the displace is reversed as one of its steps, as before - +// the separate offer exists only for the file that cannot be reversed. +func TestDisplacedFileIsNotOfferedTwice(t *testing.T) { + ents := []journal.Entry{ + {Action: "displace", Status: "ok", File: "a.pdf", Dir: "dl", Step: 1, + Src: "/archive/a.pdf", Dst: "/trash/files/a.pdf", Detail: "a.pdf"}, + {Action: "move", Status: "ok", File: "a.pdf", Dir: "dl", Step: 1, + Src: "/dl/a.pdf", Dst: "/archive/a.pdf"}, + } + if _, ok := displacedUndoFile("dl", ents, map[journal.ReversedKey]int{}); ok { + t.Error("a reversible chain's displace was offered a second time on its own") + } +} diff --git a/internal/journal/read.go b/internal/journal/read.go index aa79118..a41cd8a 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -236,6 +236,7 @@ func Runs(path string, n int) ([]Run, error) { type fileOf struct{ run, dir, file string } reversibleOf := map[fileOf]int{} deletedFile := map[fileOf]bool{} + displacedOf := map[fileOf]int{} for _, line := range lines { e, ok := parseLine(line) @@ -259,6 +260,13 @@ func Runs(path string, n int) ([]Run, error) { if e.Action == "delete" { deletedFile[fileOf{e.Run, e.Dir, e.File}] = true } + if e.Action == "displace" { + // Counted apart from the rest of its file: a permanent + // delete ends the file it deleted, but the file that chain + // trashed to make room is a different file and can still + // come back. + displacedOf[fileOf{e.Run, e.Dir, e.File}]++ + } } if e.Action == "run-start" { if orig, ok := strings.CutPrefix(e.Detail, undoOfPrefix); ok && orig != "" { @@ -298,9 +306,16 @@ func Runs(path string, n int) ([]Run, error) { if r.Undone { took := 0 for f, n := range reversibleOf { - if f.run == r.ID && !deletedFile[f] { - took += n + if f.run != r.ID { + continue + } + if deletedFile[f] { + // Nothing of a permanently deleted file comes back + // except what it displaced. + took += displacedOf[f] + continue } + took += n } r.PartlyUndone = reversed[r.ID] < took } diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index d2bbd3a..dd202a4 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -808,3 +808,52 @@ func TestRunsIgnoresAPermanentlyDeletedFilesSteps(t *testing.T) { } } } + +// TestPermanentDeleteDoesNotExcuseItsDisplace: a chain that overwrote and +// then permanently deleted trashed a file the user owned. That file can +// come back, so until it does the run is only partly undone - subtracting +// every reversible step of a permanently deleted file, its displace +// included, made krino report the run fully undone while the user's file +// was still in the Trash. +func TestPermanentDeleteDoesNotExcuseItsDisplace(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + now := time.Now() + w.Append(Entry{Time: now, Run: "A", Action: "run-start", Status: "ok"}) + w.Append(Entry{Time: now, Run: "A", Dir: "dl", File: "a.pdf", Step: 1, + Action: "displace", Status: "ok", Src: "/archive/a.pdf", Dst: "/trash/a.pdf", Detail: "a.pdf"}) + w.Append(Entry{Time: now, Run: "A", Dir: "dl", File: "a.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/dl/a.pdf", Dst: "/archive/a.pdf"}) + w.Append(Entry{Time: now, Run: "A", Dir: "dl", File: "a.pdf", Step: 2, + Action: "delete", Status: "ok", Src: "/archive/a.pdf"}) + // A second file the same run moved, which undo does put back. + w.Append(Entry{Time: now, Run: "A", Dir: "dl", File: "b.txt", Step: 1, + Action: "move", Status: "ok", Src: "/dl/b.txt", Dst: "/dl/Text/b.txt"}) + w.Append(Entry{Time: now, Run: "A", Action: "run-end", Status: "ok"}) + w.Append(Entry{Time: now.Add(time.Hour), Run: "B", Action: "run-start", Status: "ok", + Detail: undoOfPrefix + "A"}) + w.Append(Entry{Time: now.Add(time.Hour), Run: "B", Dir: "dl", File: "b.txt", Step: 1, + Action: "undo-move", Status: "ok", Src: "/dl/Text/b.txt", Dst: "/dl/b.txt"}) + w.Append(Entry{Time: now.Add(time.Hour), Run: "B", Action: "run-end", Status: "ok"}) + w.Close() + + runs, err := Runs(path, 0) + if err != nil { + t.Fatal(err) + } + var orig *Run + for i := range runs { + if runs[i].ID == "A" { + orig = &runs[i] + } + } + if orig == nil { + t.Fatal("the original run is not in the log") + } + if !orig.Undone { + t.Fatal("the run is not marked undone at all") + } + if !orig.PartlyUndone { + t.Error("a run whose displaced file is still in the Trash reads as fully undone") + } +} |