aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--cmd/krino/history_test.go76
-rw-r--r--cmd/krino/undo.go6
-rw-r--r--docs/design.md16
-rw-r--r--internal/engine/apply.go19
-rw-r--r--internal/engine/undo_identity_test.go51
-rw-r--r--internal/journal/read.go37
-rw-r--r--internal/journal/read_test.go48
-rw-r--r--man/krino.17
8 files changed, 252 insertions, 8 deletions
diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go
index 5d86b6f..1a9ea2b 100644
--- a/cmd/krino/history_test.go
+++ b/cmd/krino/history_test.go
@@ -4,6 +4,7 @@ package main
import (
"bytes"
+ "context"
"fmt"
"os"
"path/filepath"
@@ -12,6 +13,7 @@ import (
"time"
"krino/internal/engine"
+ "krino/internal/journal"
)
func TestLogListsRunsAndUndoReverses(t *testing.T) {
@@ -45,8 +47,19 @@ func TestLogListsRunsAndUndoReverses(t *testing.T) {
if _, out, _ = runCLI(t, "log"); !strings.Contains(out, "undone") {
t.Errorf("log does not mark the run undone:\n%s", out)
}
- if code, _, errOut = runCLI(t, "undo", "-y"); code == 0 {
- t.Errorf("undoing an undo run succeeded: %q", errOut)
+ // Plain undo after an undo continues the run it undid (review M10):
+ // everything came back, so nothing is left and nothing moves.
+ if code, out, errOut := runCLI(t, "undo", "-y"); code != 0 || !strings.Contains(out, "0 applied") {
+ t.Errorf("undo after a complete undo: %d\n%s\n%s", code, out, errOut)
+ }
+ if _, err := os.Stat(filepath.Join(h, "dl", "inv1.txt")); err != nil {
+ t.Errorf("the restored file moved: %v", err)
+ }
+ // Naming the undo run itself is still refused.
+ _, out, _ = runCLI(t, "log")
+ undoRun := strings.Fields(out)[0]
+ if code, _, errOut = runCLI(t, "undo", "-y", undoRun); code == 0 || !strings.Contains(errOut, "itself an undo") {
+ t.Errorf("undoing undo run %s: exit %d %q", undoRun, code, errOut)
}
}
@@ -394,3 +407,62 @@ func TestGlobalYesBeforeUndo(t *testing.T) {
t.Error("the filed copy survived -y undo")
}
}
+
+// TestUndoWithoutRunContinuesTheLastUndo: when the most recent run is an
+// undo that could not finish, plain `krino undo` offers what that undo left
+// instead of refusing because the last run is an undo (review M10).
+func TestUndoWithoutRunContinuesTheLastUndo(t *testing.T) {
+ h := home(t)
+ dl := filepath.Join(h, "dl")
+ if err := os.MkdirAll(dl, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
+ p := filepath.Join(dl, "a.pdf")
+ if err := os.WriteFile(p, []byte("one"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ os.Chtimes(p, old, old)
+ if code, _, errOut := runCLI(t, "init"); code != 0 {
+ t.Fatal(errOut)
+ }
+ if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 {
+ t.Fatal(errOut)
+ }
+ rules := "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"
+ os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644)
+ if code, out, errOut := runCLI(t, "-y"); code != 0 {
+ t.Fatalf("sort: %d\n%s\n%s", code, out, errOut)
+ }
+ // An undo that fails part way: planned, then something takes the
+ // original name before it runs (driven through the engine, since the CLI
+ // plans and applies in one go).
+ e, errs := engine.Load(filepath.Join(h, ".config", "krino", "krino.conf"))
+ if len(errs) > 0 {
+ t.Fatal(errs)
+ }
+ runs, err := e.Runs(1)
+ if err != nil || len(runs) != 1 {
+ t.Fatalf("runs: %v %v", runs, err)
+ }
+ up, err := e.PlanUndo(runs[0].ID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ os.WriteFile(p, []byte("in the way"), 0o644)
+ j, err := journal.Open(e.Config.LogFile())
+ if err != nil {
+ t.Fatal(err)
+ }
+ time.Sleep(1100 * time.Millisecond) // run ids are per second
+ res, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now()))
+ j.Close()
+ if err != nil || res.Failed != 1 {
+ t.Fatalf("blocked undo: %+v, %v", res, err)
+ }
+ os.Remove(p)
+ code, out, errOut := runCLI(t, "undo", "-n")
+ if code != 0 || !strings.Contains(out, "undo-rename") || strings.Contains(out, "undo-move") {
+ t.Fatalf("undo -n after a failed undo: exit %d\n%s\n%s", code, out, errOut)
+ }
+}
diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go
index b7a139f..c942ae1 100644
--- a/cmd/krino/undo.go
+++ b/cmd/krino/undo.go
@@ -97,6 +97,12 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int {
return 0
}
runID = runs[0].ID
+ // The most recent run is itself an undo: continue it, by planning the
+ // run it undid again - reversals it completed are not offered twice
+ // (review M10). Naming an undo run explicitly is still refused.
+ if runs[0].UndoOf != "" {
+ runID = runs[0].UndoOf
+ }
}
// PlanUndo only reads the log; nothing is touched yet (spec ยง10), which
diff --git a/docs/design.md b/docs/design.md
index 11e859e..caa2a0f 100644
--- a/docs/design.md
+++ b/docs/design.md
@@ -618,9 +618,19 @@ not logged; declined files are.
- `krino log` lists recent runs: id, time, directories, counts.
- `krino undo` reverses the most recent run, in every directory it
- touched; if that run is itself an undo run, it is refused. An older run
- is undone by naming it: `krino undo RUN`. Undo runs cannot themselves be
- undone.
+ touched. An older run is undone by naming it: `krino undo RUN`. Undo runs
+ cannot themselves be undone: naming one is refused.
+- A run is undone as far as it can be. Reversals an earlier undo of the
+ same run completed are not offered again, so an undo that stopped part
+ way (a refused or failed step, an interrupt) is finished by undoing the
+ run once more; when the most recent run is itself an undo, plain
+ `krino undo` does exactly that for the run it undid.
+- A file is identified by its directory and its path within it, so two
+ directories' files of the same name are reversed apart. A file that one
+ directory's rules move into another included directory, which sorts it
+ again in the same run, is two files to undo, reversed in the order the
+ run touched them; the first may then be refused as missing (known
+ limitation).
- Undo builds a plan like any other, shown and approved the same way
(`-y` and `-n` apply). Its per-file prompt has no `t` or `d`.
diff --git a/internal/engine/apply.go b/internal/engine/apply.go
index 94d65e4..e109aaf 100644
--- a/internal/engine/apply.go
+++ b/internal/engine/apply.go
@@ -343,6 +343,14 @@ 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)
}
+ // Reversals an earlier undo of this same run already completed are not
+ // offered again (review M10): an undo that stopped part way can be
+ // finished by undoing the run once more.
+ reversed, err := journal.ReversedSteps(e.Config.LogFile(), runID)
+ if err != nil {
+ return nil, fmt.Errorf("engine: plan undo: %w", err)
+ }
+
// 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.
@@ -362,7 +370,7 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) {
up := &UndoPlan{Run: runID}
for _, k := range order {
- uf := planUndoFile(k.dir, k.file, byFile[k])
+ uf := planUndoFile(k.dir, k.file, byFile[k], reversed)
// 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
@@ -464,7 +472,7 @@ 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(dir, file string, ents []journal.Entry) UndoFile {
+func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.ReversedKey]int) UndoFile {
uf := UndoFile{File: file, Dir: dir}
proj := newUndoProjection()
for i := len(ents) - 1; i >= 0; i-- {
@@ -483,6 +491,13 @@ func planUndoFile(dir, file string, ents []journal.Entry) UndoFile {
break
}
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.
+ reversed[k]--
+ proj.record(step)
+ continue
+ }
if (en.Action == "move" || en.Action == "rename") && proj.occupied[en.Dst] {
// A reversal already queued for this same file puts it back at
// en.Dst before this one runs, and that reversal was checked
diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go
index 9c93b3b..d4169be 100644
--- a/internal/engine/undo_identity_test.go
+++ b/internal/engine/undo_identity_test.go
@@ -223,3 +223,54 @@ func TestUndoLeavesNoDirectoriesBehind(t *testing.T) {
t.Errorf("dl/Out is left behind: %v", err)
}
}
+
+// TestUndoCanBeFinishedAfterAFailure: an undo whose last reversal failed
+// (something took the original name) leaves the file part way back; once
+// the obstacle is gone, undoing the same run again offers only the step
+// that is left, and finishes it (review M10).
+func TestUndoCanBeFinishedAfterAFailure(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)
+ }
+ blocker := filepath.Join(h, "dl", "a.pdf")
+ if err := os.WriteFile(blocker, []byte("in the way"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ undo := func(up *UndoPlan) *ApplyResult {
+ t.Helper()
+ j, err := journal.Open(logPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer j.Close()
+ res, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ return res
+ }
+ if res := undo(up); res.Failed != 1 {
+ t.Fatalf("first undo: Failed = %d, want 1 (the rename back is blocked)", res.Failed)
+ }
+ if err := os.Remove(blocker); err != nil {
+ t.Fatal(err)
+ }
+ time.Sleep(10 * time.Millisecond) // a new run id
+ again, err := e.PlanUndo(run)
+ if err != nil {
+ t.Fatal(err)
+ }
+ f := undoFileNamed(t, again, "dl", "a.pdf")
+ if f.Refused != "" || len(f.Steps) != 1 || f.Steps[0].Action != "undo-rename" {
+ t.Fatalf("second plan: %+v; want one undo-rename step", f)
+ }
+ if res := undo(again); res.Failed != 0 || res.Applied != 1 {
+ t.Fatalf("second undo: %+v", res)
+ }
+ if b, err := os.ReadFile(filepath.Join(h, "dl", "a.pdf")); err != nil || string(b) != "one" {
+ t.Errorf("dl/a.pdf = %q, %v; want the original back", b, err)
+ }
+}
diff --git a/internal/journal/read.go b/internal/journal/read.go
index 3de15d2..de06422 100644
--- a/internal/journal/read.go
+++ b/internal/journal/read.go
@@ -43,6 +43,42 @@ type Run struct {
Dirs []string
Counts map[string]int // action -> count of status "ok"
Undone bool // a later run reversed this one
+ UndoOf string // for an undo run, the run it reverses; "" otherwise
+}
+
+// ReversedKey identifies one reversal an undo run carried out: the file's
+// directory and name, the undo action and the path it started from - enough
+// to tell which step of the original run it reversed.
+type ReversedKey struct {
+ Dir, File, Action, Src string
+}
+
+// ReversedSteps counts, for runID, every reversal that earlier undo runs of
+// it completed ("ok" undo- entries of runs whose run-start says they undo
+// runID), so a later undo of the same run can offer only what is left
+// (review M10). An undo run's own unparsable lines are skipped; a missing
+// reversal is then offered again, where its own checks refuse it if it had
+// in fact happened.
+func ReversedSteps(path, runID string) (map[ReversedKey]int, error) {
+ lines, err := readLines(path)
+ if err != nil {
+ return nil, err
+ }
+ undoRuns := map[string]bool{}
+ for _, line := range lines {
+ if e, ok := parseLine(line); ok && e.Action == "run-start" && e.Detail == UndoOf(runID) {
+ undoRuns[e.Run] = true
+ }
+ }
+ out := map[ReversedKey]int{}
+ for _, line := range lines {
+ e, ok := parseLine(line)
+ if !ok || !undoRuns[e.Run] || e.Status != "ok" || !strings.HasPrefix(e.Action, "undo-") {
+ continue
+ }
+ out[ReversedKey{Dir: e.Dir, File: e.File, Action: e.Action, Src: e.Src}]++
+ }
+ return out, nil
}
// Entries returns every entry belonging to runID, in file order. A line
@@ -198,6 +234,7 @@ func Runs(path string, n int) ([]Run, error) {
sort.SliceStable(runs, func(i, j int) bool { return runs[i].Start.After(runs[j].Start) })
for i := range runs {
runs[i].Undone = undoes[runs[i].ID]
+ runs[i].UndoOf = pendingUndo[runs[i].ID]
}
if n > 0 && n < len(runs) {
diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go
index 2f0f40f..2b46bfb 100644
--- a/internal/journal/read_test.go
+++ b/internal/journal/read_test.go
@@ -625,3 +625,51 @@ func TestEntriesFailsClosedOnMissingRunStart(t *testing.T) {
t.Errorf("entries = %+v", got)
}
}
+
+// TestReversedStepsCountsEveryUndoOfARun: the steps earlier undo runs of a
+// run already reversed - only "ok" undo entries of runs that undo it - so
+// a later undo of the same run can offer just what is left (review M10).
+// Runs also names the run an undo run reversed.
+func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "krino.log")
+ w, err := Open(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ at := time.Date(2026, 9, 11, 9, 0, 0, 0, time.UTC)
+ for _, e := range []Entry{
+ {Time: at, Run: "R", Action: "run-start", Status: "ok"},
+ {Time: at, Run: "R", Dir: "dl", File: "a.pdf", Step: 1, Action: "move", Status: "ok", Src: "/dl/a.pdf", Dst: "/w/a.pdf"},
+ {Time: at, Run: "R", Action: "run-end", Status: "ok"},
+ {Time: at.Add(time.Minute), Run: "U1", Action: "run-start", Status: "ok", Detail: UndoOf("R")},
+ {Time: at.Add(time.Minute), Run: "U1", Dir: "dl", File: "a.pdf", Step: 1, Action: "undo-move", Status: "ok", Src: "/w/a.pdf", Dst: "/dl/a.pdf"},
+ {Time: at.Add(time.Minute), Run: "U1", Dir: "dl", File: "b.pdf", Step: 1, Action: "undo-rename", Status: "failed", Src: "/dl/r-b.pdf", Dst: "/dl/b.pdf"},
+ {Time: at.Add(time.Minute), Run: "U1", Action: "run-end", Status: "ok"},
+ {Time: at.Add(2 * time.Minute), Run: "U2", Action: "run-start", Status: "ok", Detail: UndoOf("OTHER")},
+ {Time: at.Add(2 * time.Minute), Run: "U2", Dir: "dl", File: "a.pdf", Step: 1, Action: "undo-move", Status: "ok", Src: "/w/a.pdf", Dst: "/dl/a.pdf"},
+ {Time: at.Add(2 * time.Minute), Run: "U2", Action: "run-end", Status: "ok"},
+ } {
+ if err := w.Append(e); err != nil {
+ t.Fatal(err)
+ }
+ }
+ w.Close()
+ got, err := ReversedSteps(path, "R")
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := map[ReversedKey]int{{Dir: "dl", File: "a.pdf", Action: "undo-move", Src: "/w/a.pdf"}: 1}
+ if len(got) != len(want) || got[ReversedKey{Dir: "dl", File: "a.pdf", Action: "undo-move", Src: "/w/a.pdf"}] != 1 {
+ t.Errorf("ReversedSteps = %v, want %v", got, want)
+ }
+ runs, err := Runs(path, 0)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, r := range runs {
+ want := map[string]string{"R": "", "U1": "R", "U2": "OTHER"}[r.ID]
+ if r.UndoOf != want {
+ t.Errorf("run %s: UndoOf = %q, want %q", r.ID, r.UndoOf, want)
+ }
+ }
+}
diff --git a/man/krino.1 b/man/krino.1
index e7d93eb..c0bba9d 100644
--- a/man/krino.1
+++ b/man/krino.1
@@ -208,7 +208,9 @@ Reverse
.Ar run .
With no
.Ar run ,
-reverse the most recent run; if that run is itself an undo, it is refused.
+reverse the most recent run; if that run is itself an undo, continue it:
+the run it undid is planned again, offering only the reversals it did not
+complete.
An older run is undone by naming it.
See
.Sx UNDO .
@@ -311,6 +313,9 @@ and
An undo run cannot itself be undone: naming it to
.Ic undo
is refused.
+Reversals an earlier undo of the same run completed are never offered
+again, so an undo that stopped part way is finished by undoing the run
+once more.
.Pp
A step's reversal is refused when the world has moved on since the step ran
.Pq its target is gone or has changed, or the original path is occupied again ;
generated by cgit v1.3 (git 2.53.0) at 2026-09-19 11:10:14 +0000