From 643eac4a2cba7b34b8a33af1b3dbdc54d9816a53 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 14 Sep 2026 21:30:49 +0200 Subject: plan 9: undo checks trash identity, groups by directory, re-checks at execution, removes directories it recreates --- internal/engine/apply.go | 98 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 28 deletions(-) (limited to 'internal/engine/apply.go') diff --git a/internal/engine/apply.go b/internal/engine/apply.go index 83ea4fe..94d65e4 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -343,21 +343,26 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { return nil, fmt.Errorf("engine: plan undo: run %s is itself an undo and cannot be undone", runID) } - var order []string - byFile := map[string][]journal.Entry{} + // Entries are grouped by directory and file together (review M7): one run + // spans every directory, and two directories can each hold a file of the + // same name. + type fileKey struct{ dir, file string } + var order []fileKey + byFile := map[fileKey][]journal.Entry{} for _, en := range entries { if en.File == "" { // run-start / run-end continue } - if _, seen := byFile[en.File]; !seen { - order = append(order, en.File) + k := fileKey{en.Dir, en.File} + if _, seen := byFile[k]; !seen { + order = append(order, k) } - byFile[en.File] = append(byFile[en.File], en) + byFile[k] = append(byFile[k], en) } up := &UndoPlan{Run: runID} - for _, file := range order { - uf := planUndoFile(file, byFile[file]) + for _, k := range order { + uf := planUndoFile(k.dir, k.file, byFile[k]) // Critical finding, Task 8's review: a file every one of whose // entries has Status != "ok" (declined by the ORIGINAL run's own // review, or skipped, or failed before anything happened) yields @@ -459,8 +464,8 @@ func isFileAffecting(action string) bool { // two steps of one file that touch the same path, and every un-contended // check keeps behaving exactly as before, since the projection only ever // overrides a real occupant that this same chain is itself about to clear. -func planUndoFile(file string, ents []journal.Entry) UndoFile { - uf := UndoFile{File: file, Dir: dirOf(ents)} +func planUndoFile(dir, file string, ents []journal.Entry) UndoFile { + uf := UndoFile{File: file, Dir: dir} proj := newUndoProjection() for i := len(ents) - 1; i >= 0; i-- { en := ents[i] @@ -560,17 +565,6 @@ func needsOccupancyCheck(action string) bool { return false } -// dirOf returns the first non-empty Dir among ents, which should all agree -// since one file is always processed within one configured directory. -func dirOf(ents []journal.Entry) string { - for _, en := range ents { - if en.Dir != "" { - return en.Dir - } - } - return "" -} - // reverseStep computes the UndoStep for one logged "ok" entry, per spec // ยง10's reversal table. It only stats the filesystem to decide Refused (the // "changed since" and "trash entry is gone" checks); it never mutates @@ -597,9 +591,7 @@ func reverseStep(en journal.Entry) UndoStep { us.Refused = refuseIfChanged(en) case "trash", "displace": us.Src, us.Dst = en.Dst, en.Src - if _, err := os.Stat(en.Dst); err != nil { - us.Refused = "the trash entry is gone" - } + us.Refused = refuseIfTrashChanged(en) case "mkdir": us.Src = en.Dst } @@ -635,6 +627,38 @@ func refuseIfChanged(en journal.Entry) string { return "" } +// refuseIfTrashChanged is the trash reversal's identity check (review M2): +// the entry must still be the file this run put there - the size and mtime +// the run logged for it - and its trashinfo must still record the path it +// came from. Emptying the Trash and trashing another file of the same name +// would otherwise have undo restore that file instead. +func refuseIfTrashChanged(en journal.Entry) string { + fi, err := os.Lstat(en.Dst) + if err != nil { + return "the trash entry is gone" + } + if fi.Size() != en.Size || !fi.ModTime().Equal(en.ModTime) { + return fmt.Sprintf("the trash entry %s is not the file this run put there", en.Detail) + } + if p, err := trash.InfoPath(en.Detail); err != nil || p != en.Src { + return fmt.Sprintf("the trash entry %s now belongs to another file", en.Detail) + } + return "" +} + +// recheck repeats, at execution time, the identity check PlanUndo made +// (review undo F8): an undo plan is shown and approved first, and a file +// changed in that window must not be moved back or trashed. +func recheck(step UndoStep) string { + switch step.Action { + case "undo-move", "undo-rename", "undo-copy": + return refuseIfChanged(step.Original) + case "undo-trash", "undo-displace": + return refuseIfTrashChanged(step.Original) + } + return "" +} + // refuseIfSrcExists is the second half of every "reversal puts a file back // at a fixed path" refusal condition: reversing would silently clobber // whatever is there now. Fix wave item 1: it is no longer reverseStep's own @@ -773,7 +797,10 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, tallyFile(result, fr.Steps, func(i int) bool { return isFileAffecting(f.Steps[i].Action) }) for i, us := range f.Steps { if us.Action == "undo-mkdir" && fr.Steps[i].Status == "failed" { - retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1}) + retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1, log: true}) + } + for _, made := range fr.Steps[i].Made { + retries = append(retries, dirRetry{dir: made}) } } } @@ -802,6 +829,10 @@ type dirRetry struct { dirName string file string step int + // log is false for a directory this undo run itself created on the way + // (runUndoStep's Made): the original run's log already says it was + // removed, so removing it again needs no entry of its own. + log bool } // retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once @@ -833,6 +864,9 @@ func (e *Engine) retryDirRemovals(j *journal.Writer, run string, candidates []di // original refusal already recorded this, and it stands. continue } + if !c.log { + continue + } if err := j.Append(journal.Entry{ Time: e.Now(), Run: run, Dir: c.dirName, File: c.file, Step: c.step, Action: "undo-mkdir", Status: "ok", Src: c.dir, @@ -924,16 +958,24 @@ func (e *Engine) undoFile(f UndoFile, j *journal.Writer, run string) (FileResult // Step field does not apply here (there is no plan.Kind for an undo) and is // left zero. func runUndoStep(step UndoStep) apply.StepResult { + if why := recheck(step); why != "" { + return apply.StepResult{Status: "failed", Detail: why} + } switch step.Action { case "undo-move", "undo-rename": - if err := os.MkdirAll(filepath.Dir(step.Dst), 0o755); err != nil { - return apply.StepResult{Status: "failed", Detail: err.Error()} + // The directories created here are returned in Made: ApplyUndo removes + // them again once empty, since an earlier file's undo-mkdir may already + // have removed the directory this file passes back through (review + // undo F6). + made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst)) + if err != nil { + return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} } if err := renameOrCopy(step.Src, step.Dst); err != nil { - return apply.StepResult{Status: "failed", Detail: err.Error()} + return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} } size, mtime := statSizeModTime(step.Dst) - return apply.StepResult{Status: "ok", Dst: step.Dst, Size: size, ModTime: mtime} + return apply.StepResult{Status: "ok", Dst: step.Dst, Size: size, ModTime: mtime, Made: made} case "undo-copy": entry, err := trash.Put(step.Src) -- cgit v1.3