plan 10: resumed undo re-checks what an earlier undo put back; occupied directory removals are not offered forever

2 files changed, 98 insertions, 2 deletions

diff --git a/internal/engine/apply.go b/internal/engine/apply.go
index ed2fbe5..75248fb 100644
--- a/internal/engine/apply.go
+++ b/internal/engine/apply.go
@@ -409,11 +409,32 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) {
 		if len(uf.Steps) == 0 && uf.Refused == "" {
 			continue
 		}
+		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
+			// would repeat on every undo (re-review undo F3). An empty one is
+			// still offered, and removed.
+			continue
+		}
 		up.Files = append(up.Files, uf)
 	}
 	return up, nil
 }
 
+// onlyOccupiedDirectoryRemovals reports whether every step is an undo-mkdir
+// of a directory that is not empty now, so none of them could run.
+func onlyOccupiedDirectoryRemovals(steps []UndoStep) bool {
+	for _, s := range steps {
+		if s.Action != "undo-mkdir" {
+			return false
+		}
+		if entries, err := os.ReadDir(s.Src); err == nil && len(entries) == 0 {
+			return false
+		}
+	}
+	return true
+}
+
 // isUndoRun reports whether every one of entries' file-scoped actions is
 // already an "undo-" action, i.e. entries belongs to a run ApplyUndo itself
 // produced. Runs cannot themselves be undone (spec §10). This does not read
@@ -523,9 +544,11 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R
 		step := reverseStep(en)
 		if k := (journal.ReversedKey{Dir: dir, File: file, Action: step.Action, Src: step.Src}); reversed[k] > 0 {
 			// An earlier undo of this run already reversed this step: the
-			// disk already shows it, and it is not offered again.
+			// disk already shows it, and it is not offered again. It is not
+			// recorded in the projection either (re-review undo F1): what it
+			// put back is on disk now and is checked there, so a file changed
+			// since is refused rather than vouched for by the old reversal.
 			reversed[k]--
-			proj.record(step)
 			continue
 		}
 		if (en.Action == "move" || en.Action == "rename") && proj.occupied[en.Dst] {
diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go
index 5040ece..7de4eb9 100644
--- a/internal/engine/undo_identity_test.go
+++ b/internal/engine/undo_identity_test.go
@@ -359,3 +359,76 @@ func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) {
 		t.Errorf("a.pdf not restored: %q %v", b, err)
 	}
 }
+
+// TestResumedUndoStillRefusesAChangedFile: an undo that stopped after its
+// first reversal must not let that reversal vouch for the file later: the
+// file edited in between is refused when the undo is resumed (re-review
+// undo F1).
+func TestResumedUndoStillRefusesAChangedFile(t *testing.T) {
+	e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"})
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	first := undoFileNamed(t, up, "dl", "a.pdf")
+	if len(first.Steps) < 2 || first.Steps[0].Action != "undo-move" {
+		t.Fatalf("unexpected plan: %+v", first)
+	}
+	// An undo that ran only its first reversal, then stopped.
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	undoRun := journal.NewRunID(time.Now().Add(time.Second))
+	if err := j.Append(journal.Entry{Time: time.Now(), Run: undoRun, Action: "run-start", Status: "ok", Detail: journal.UndoOf(run)}); err != nil {
+		t.Fatal(err)
+	}
+	partial := first
+	partial.Steps = first.Steps[:1]
+	if _, err := e.undoFile(partial, j, undoRun); err != nil {
+		t.Fatal(err)
+	}
+	j.Close()
+	// The file, back at its renamed name, is edited before the undo resumes.
+	if err := os.WriteFile(filepath.Join(h, "dl", "r-a.pdf"), []byte("edited in between"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	again, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if f := undoFileNamed(t, again, "dl", "a.pdf"); f.Refused == "" {
+		t.Errorf("the edited file is offered again: %+v", f)
+	}
+}
+
+// TestUndoDoesNotOfferOnlyADirectoryRemoval: when a directory the run made
+// still holds a file of the user's, the restored file's remaining
+// directory removal is not offered on every later undo (re-review undo F3).
+func TestUndoDoesNotOfferOnlyADirectoryRemoval(t *testing.T) {
+	e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"})
+	if err := os.WriteFile(filepath.Join(h, "dl", "Out", "notes.txt"), []byte("mine"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	up, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	j, err := journal.Open(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now())); err != nil {
+		t.Fatal(err)
+	}
+	j.Close()
+	again, err := e.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(again.Files) != 0 {
+		t.Errorf("after a complete undo, still offered: %+v", again.Files)
+	}
+}