diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-12 20:14:47 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-12 20:14:47 +0200 |
| commit | 3f8679be9373ee7508d512dfdfc1dda0839c7f90 (patch) | |
| tree | ec02eb075f6c4e90f21baa2fe674e86a2f7f6a62 /cmd/krino | |
| parent | 24a84671ace373ae331fa83a1ff484990f4dff0e (diff) | |
| download | krino-3f8679be9373ee7508d512dfdfc1dda0839c7f90.tar.gz krino-3f8679be9373ee7508d512dfdfc1dda0839c7f90.zip | |
krino: acting — trash, journal, apply, lock, review, undo
Diffstat (limited to 'cmd/krino')
| -rw-r--r-- | cmd/krino/commands_test.go | 23 | ||||
| -rw-r--r-- | cmd/krino/history_test.go | 317 | ||||
| -rw-r--r-- | cmd/krino/log.go | 122 | ||||
| -rw-r--r-- | cmd/krino/main.go | 14 | ||||
| -rw-r--r-- | cmd/krino/main_test.go | 8 | ||||
| -rw-r--r-- | cmd/krino/matching_test.go | 99 | ||||
| -rw-r--r-- | cmd/krino/render.go | 26 | ||||
| -rw-r--r-- | cmd/krino/review.go | 180 | ||||
| -rw-r--r-- | cmd/krino/review_test.go | 144 | ||||
| -rw-r--r-- | cmd/krino/sort.go | 356 | ||||
| -rw-r--r-- | cmd/krino/sort_test.go | 91 | ||||
| -rw-r--r-- | cmd/krino/undo.go | 533 |
12 files changed, 1863 insertions, 50 deletions
diff --git a/cmd/krino/commands_test.go b/cmd/krino/commands_test.go index 2156da2..b8364e6 100644 --- a/cmd/krino/commands_test.go +++ b/cmd/krino/commands_test.go @@ -9,7 +9,13 @@ import ( "testing" ) -// home gives each test its own HOME with no XDG overrides. +// home gives each test its own HOME with no XDG overrides, and points the +// package's stdin seam at something guaranteed non-terminal (fix round +// 2026-09-12/item 4): every test that reaches cmdSort's terminal check or +// the interactive review must not depend on what the ambient test binary's +// stdin happens to be - if that were ever a real terminal, such a test +// would silently fall through to the interactive prompt and block on a +// keypress instead of failing. func home(t *testing.T) string { t.Helper() h := t.TempDir() @@ -17,9 +23,24 @@ func home(t *testing.T) string { for _, v := range []string{"XDG_CONFIG_HOME", "XDG_STATE_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME"} { t.Setenv(v, "") } + setNonTerminalStdin(t) return h } +// setNonTerminalStdin points the stdin seam (sort.go) at os.DevNull for the +// duration of the calling test, restoring it afterward. +func setNonTerminalStdin(t *testing.T) { + t.Helper() + f, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { f.Close() }) + old := stdin + stdin = f + t.Cleanup(func() { stdin = old }) +} + func TestInitNewCheck(t *testing.T) { h := home(t) if err := os.Mkdir(filepath.Join(h, "dl"), 0o755); err != nil { diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go new file mode 100644 index 0000000..2667b63 --- /dev/null +++ b/cmd/krino/history_test.go @@ -0,0 +1,317 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "krino/internal/engine" +) + +func TestLogListsRunsAndUndoReverses(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("nothing was filed: %v", err) + } + + code, out, errOut := runCLI(t, "log") + if code != 0 { + t.Fatalf("log: %d %s", code, errOut) + } + if !strings.Contains(out, "moved") || !strings.Contains(out, "dl") { + t.Errorf("log output:\n%s", out) + } + + if code, _, errOut = runCLI(t, "undo", "-y"); code != 0 { + t.Fatalf("undo: %d %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, "dl", "inv1.txt")); err != nil { + t.Errorf("undo did not put the file back: %v", err) + } + if _, err := os.Stat(filed); !os.IsNotExist(err) { + t.Error("the filed copy survived the undo") + } + + 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) + } +} + +func TestUndoDryRunChangesNothing(t *testing.T) { + h := matchingFixture(t) + runCLI(t, "-y") + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if code, out, _ := runCLI(t, "undo", "-n"); code != 0 || !strings.Contains(out, "undo-move") { + t.Errorf("undo -n: %d\n%s", code, out) + } + if _, err := os.Stat(filed); err != nil { + t.Error("undo -n moved a file") + } +} + +// TestUndoFailsImmediatelyWithHeldLock is fix round 2026-09-12, item 1: an +// undo moves files just as an apply does, so it needs the same per-directory +// guard sort's TestSecondRunFailsImmediatelyWithYes already pins for the +// forward path (spec §11: "a second krino on the same directory ... fails +// immediately with -y"). The lock file is held under the config NAME "dl", +// not any filesystem path - UndoFile.Dir is the journal's `dir` column, +// which is the directory's name from krino.conf, not its root. +// +// Ruling (fix round 2026-09-12, follow-up): the lock is acquired before the +// plan is even shown, matching cmdSort's own window (acquired before +// Plan/review, held across both) rather than only around ApplyUndo - so +// this also asserts the refusal is noticed before any plan output reaches +// stdout. A version of this test that only checked the exit code and +// stderr would pass equally whether the lock were taken early or late, and +// so would not be pinning the thing this ruling is actually about. +func TestUndoFailsImmediatelyWithHeldLock(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + filed := filepath.Join(h, "dl", "Work", "Acme", "inv1.txt") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("nothing was filed: %v", err) + } + + held := filepath.Join(h, ".local", "state", "krino", "dl.lock") + if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil { + t.Fatal(err) + } + + code, out, errOut := runCLI(t, "undo", "-y") + if code != 1 || !strings.Contains(errOut, "another krino") { + t.Errorf("undo -y against a held lock: %d %q", code, errOut) + } + if strings.Contains(out, "to reverse") || strings.Contains(out, "undo-move") { + t.Errorf("undo printed the plan before noticing the held lock:\n%s", out) + } + if _, err := os.Stat(filed); err != nil { + t.Errorf("undo reversed the file despite the held lock: %v", err) + } +} + +// undoFiles builds minimal engine.UndoFile fixtures for reviewUndoFiles/ +// reviewUndoPerFile, named by rel path only (Dir left blank - the tests +// below never render it). +func undoFiles(rels ...string) []engine.UndoFile { + out := make([]engine.UndoFile, len(rels)) + for i, r := range rels { + out[i] = engine.UndoFile{File: r, Steps: []engine.UndoStep{{Action: "undo-move", Src: "/t/" + r, Dst: "/s/" + r}}} + } + return out +} + +// TestFinalizeUndoPlanMarksUnapprovedAsDeclined is the wiring point for fix +// round 2026-09-12, item 2: a refused file rides through untouched (its own +// Refused reason is what ApplyUndo checks first), an approved file rides +// through untouched too, and anything else - explicitly declined, or never +// reached because [d]/[q] cut a per-file review short - comes out with +// Declined set rather than being dropped from the plan. +func TestFinalizeUndoPlanMarksUnapprovedAsDeclined(t *testing.T) { + up := &engine.UndoPlan{Run: "r1", Files: []engine.UndoFile{ + {File: "a"}, // index 0: approved + {File: "b"}, // index 1: not approved -> declined + {File: "c", Refused: "gone"}, // index 2: refused, never declined + }} + out := finalizeUndoPlan(up, map[int]bool{0: true}) + if len(out.Files) != 3 { + t.Fatalf("files = %+v, want all three carried through", out.Files) + } + if out.Files[0].Declined || out.Files[0].Refused != "" { + t.Errorf("approved file changed: %+v", out.Files[0]) + } + if !out.Files[1].Declined || out.Files[1].Refused != "" { + t.Errorf("unapproved file not marked declined: %+v", out.Files[1]) + } + if out.Files[2].Declined { + t.Errorf("a refused file must not also be marked declined: %+v", out.Files[2]) + } + if out.Files[2].Refused != "gone" { + t.Errorf("refused file's reason changed: %+v", out.Files[2]) + } +} + +// TestReviewUndoApplyAll: [a] approves every reversible file by index. +func TestReviewUndoApplyAll(t *testing.T) { + approved, action, err := reviewUndoFiles(strings.NewReader("a"), new(strings.Builder), undoFiles("a", "b")) + if err != nil { + t.Fatal(err) + } + if action != 'a' || len(approved) != 2 || !approved[0] || !approved[1] { + t.Errorf("approved = %v action = %q; want both approved", approved, action) + } +} + +// TestReviewUndoSkipAndQuit: [s] and [q] both approve nothing. +func TestReviewUndoSkipAndQuit(t *testing.T) { + approved, action, _ := reviewUndoFiles(strings.NewReader("s"), new(strings.Builder), undoFiles("a", "b")) + if action != 's' || len(approved) != 0 { + t.Errorf("[s] = %q %v; want nothing approved", action, approved) + } + approved, action, _ = reviewUndoFiles(strings.NewReader("q"), new(strings.Builder), undoFiles("a", "b")) + if action != 'q' || len(approved) != 0 { + t.Errorf("[q] = %q %v; want nothing approved", action, approved) + } +} + +// TestReviewUndoChoosePerFile: [c] then per-file y/n, keyed by index. +func TestReviewUndoChoosePerFile(t *testing.T) { + approved, action, err := reviewUndoFiles(strings.NewReader("cyn"), new(strings.Builder), undoFiles("a", "b")) + if err != nil { + t.Fatal(err) + } + if action != 'c' || !approved[0] || approved[1] { + t.Errorf("approved = %v action = %q; want only index 0", approved, action) + } +} + +// TestReviewUndoRefusedFileNotPrompted is spec §10: a file PlanUndo already +// refused is shown (with its reason - covered end to end by +// TestUndoRefusesChangedDestination-style flows through cmdUndo) but never +// asked about, so a [c] session reading one key per file must not stall +// waiting for a key that reviewUndoPerFile never asks for. Three files, one +// key each for the two reversible ones ("y", "n"), none for the refused +// middle one: if it were prompted, the second key ("n") would answer for it +// instead of the third file, and this test would see index 2 approved +// instead of unset. +func TestReviewUndoRefusedFileNotPrompted(t *testing.T) { + files := undoFiles("a", "b", "c") + files[1].Refused = "b changed since the run" + out := new(strings.Builder) + approved, action, err := reviewUndoFiles(strings.NewReader("cyn"), out, files) + if err != nil { + t.Fatal(err) + } + if action != 'c' || !approved[0] || approved[1] || approved[2] { + t.Errorf("approved = %v action = %q; want only index 0 (1 is refused, 2 never reached)", approved, action) + } + if !strings.Contains(out.String(), "b changed since the run") { + t.Errorf("refused file's reason not shown:\n%s", out) + } +} + +// TestPrintUndoPlan is fix wave item 3 (Important), rebuilding fix round +// 2026-09-12's own golden test: that version hand-built its UndoSteps, +// including a Dst on the undo-copy step the real code never sets (Dst is +// deliberately left "" - trash.Put only chooses the entry name at execution +// time), so it was structurally incapable of catching the bug it was meant +// to guard against - an undo-copy row rendering as a bare +// "undo-copy → " with nothing said about what it would do to the +// user's backup copy, the single most destructive step an undo plan takes. +// The same lesson as Task 9's review: a hand-assembled fixture hides a test +// that cannot detect a broken copy-undo. This version runs a REAL forward +// apply (copy then move, so mkdir, copy and move all appear in one file's +// own chain) and a REAL PlanUndo, editing one file's result afterward so +// the plan also carries a genuinely refused row, then renders that. +func TestPrintUndoPlan(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(filepath.Join(dl, "Work"), 0o755); err != nil { + t.Fatal(err) + } + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, n := range []string{"inv1.pdf", "notes.pdf"} { + p := filepath.Join(dl, n) + if err := os.WriteFile(p, []byte("content of "+n), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + if code, _, errOut := runCLI(t, "init"); code != 0 { + t.Fatal(errOut) + } + if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 { + t.Fatal(errOut) + } + // "keep" mixes all three action kinds a single file's own chain can + // carry: copy needs a fresh ~/backup (one mkdir), move lands in the + // pre-created Work (no mkdir of its own). + rules := "(path \"~/dl\")\n(min-age 0s)\n(rule \"keep\" (when (type pdf)) (copy \"~/backup\") (move \"Work\"))\n" + if err := os.WriteFile(filepath.Join(h, ".config/krino/dirs/dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + if code, _, errOut := runCLI(t, "-y"); code != 0 { + t.Fatalf("apply: %d %s", code, errOut) + } + + // notes.pdf's moved copy is edited after the run, so PlanUndo genuinely + // refuses its reversal - the row this exercises must still say + // something true, never render blank. + moved := filepath.Join(dl, "Work", "notes.pdf") + if err := os.WriteFile(moved, []byte("edited since the run"), 0o644); err != nil { + t.Fatal(err) + } + + 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, err = %v", runs, err) + } + up, err := e.PlanUndo(runs[0].ID) + if err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + printUndoPlan(&buf, up) + out := buf.String() + + for _, want := range []string{ + "2 files · 1 to reverse · 1 refused\n", + "undo-move → ~/dl/inv1.pdf\n", + "undo-copy ~/backup/inv1.pdf → trash\n", + "undo-mkdir ~/backup\n", + // Minor 4 / fix wave item 5: the refusal reason must be abbreviated + // against $HOME exactly like every step cell above it, not printed + // as a raw absolute path. + "refused: ~/dl/Work/notes.pdf changed since the run\n", + } { + if !strings.Contains(out, want) { + t.Errorf("output lacks %q:\n%s", want, out) + } + } + if strings.Contains(out, "undo-copy → ") { + t.Errorf("undo-copy rendered a blank destination:\n%s", out) + } + if strings.Contains(out, h) { + t.Errorf("output leaked a raw absolute path instead of abbreviating against $HOME:\n%s", out) + } +} + +// TestReviewUndoInvalidKeyReprompts mirrors review_test.go's +// TestInvalidKeyReprompts for the undo-specific menu. +func TestReviewUndoInvalidKeyReprompts(t *testing.T) { + out := new(strings.Builder) + approved, action, err := reviewUndoFiles(strings.NewReader("zs"), out, undoFiles("a")) + if err != nil { + t.Fatal(err) + } + if action != 's' || len(approved) != 0 { + t.Errorf("approved = %v action = %q; want [s] after the bad key", approved, action) + } + if !strings.Contains(out.String(), "z") { + t.Errorf("no mention of the rejected key:\n%s", out) + } +} diff --git a/cmd/krino/log.go b/cmd/krino/log.go new file mode 100644 index 0000000..6eb15d6 --- /dev/null +++ b/cmd/krino/log.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "errors" + "fmt" + "io" + "os" + "strings" + + "krino/internal/engine" + "krino/internal/journal" +) + +func init() { commands["log"] = cmdLog } + +// pastTense renders one of the log's own action words (spec §9's wire +// format - journal.Run.Counts is keyed by these exact strings) as the past +// tense krino log displays. Those strings are never renamed to suit the +// display: krino undo parses them back out of the log, so this map is a +// display-only translation, not a second source of truth. Two families of +// action are deliberately absent, and so never appear in a run's counts +// line at all: run-start/run-end (bookkeeping, not something done to a +// file) and mkdir/undo-mkdir (a side effect of another step, not an +// outcome the user asked for). +var pastTense = map[string]string{ + "copy": "copied", + "move": "moved", + "rename": "renamed", + "trash": "trashed", + "delete": "deleted", + "displace": "displaced", + "undo-copy": "undo-copied", + "undo-move": "undo-moved", + "undo-rename": "undo-renamed", + "undo-trash": "undo-trashed", + "undo-displace": "undo-displaced", +} + +// countOrder is the fixed order krino log renders a run's counts in. +// journal.Run.Counts is a map, whose iteration order is random; a listing +// that reshuffled its own columns between two invocations of the same +// command would be unreadable. +var countOrder = []string{ + "copy", "move", "rename", "trash", "delete", "displace", + "undo-copy", "undo-move", "undo-rename", "undo-trash", "undo-displace", +} + +// cmdLog lists recent runs, newest first (spec §11: "krino log [-n N]"). +func cmdLog(g *globals, args []string, stdout, stderr io.Writer) int { + fs := flagSet("log", g) + n := fs.Int("n", 10, "") + if code, ok := parse(fs, args, stdout, stderr); !ok { + return code + } + if fs.NArg() > 0 { + return usageError(stderr, "usage: krino log [-n N]") + } + + e, errs := engine.Load(mainFile(g)) + if len(errs) > 0 { + printDiags(stderr, errs) + return 2 + } + + runs, err := e.Runs(*n) + if err != nil { + // journal.Runs (via Engine.Runs) fails closed on a genuine read + // error - permissions, a corrupt file - and that must stay + // distinguishable from the ordinary "no log yet" case below rather + // than collapsing into the same message (dispatch notes). + if errors.Is(err, os.ErrNotExist) { + fmt.Fprintln(stdout, "nothing logged yet") + return 0 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + // A log file can exist and still hold no runs (a real run with nothing + // actionable still opens the journal - dispatch notes). That is just as + // ordinary as no log file at all: same message, same exit 0, and no + // table header is printed over zero rows. + if len(runs) == 0 { + fmt.Fprintln(stdout, "nothing logged yet") + return 0 + } + + for _, r := range runs { + fmt.Fprintln(stdout, formatRun(r)) + } + return 0 +} + +// formatRun renders one journal.Run as krino log lists it: id, start time, +// the directories it touched, and its counts (see countsText), with +// "(undone)" appended when a later run has reversed it. +func formatRun(r journal.Run) string { + line := fmt.Sprintf("%s %s %s %s", r.ID, r.Start.Format("2006-01-02 15:04"), strings.Join(r.Dirs, ", "), countsText(r.Counts)) + if r.Undone { + line += " (undone)" + } + return line +} + +// countsText renders a run's counts in countOrder, past tense, skipping any +// action with no "ok" entries. "nothing applied" covers a run every one of +// whose files was declined or failed before anything ran - Counts only +// tallies "ok" entries, so such a run's map holds nothing this function +// would otherwise print. +func countsText(counts map[string]int) string { + var parts []string + for _, action := range countOrder { + if n := counts[action]; n > 0 { + parts = append(parts, fmt.Sprintf("%d %s", n, pastTense[action])) + } + } + if len(parts) == 0 { + return "nothing applied" + } + return strings.Join(parts, " · ") +} diff --git a/cmd/krino/main.go b/cmd/krino/main.go index 9618f6c..e204b5f 100644 --- a/cmd/krino/main.go +++ b/cmd/krino/main.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "os" + "strings" ) // version is stamped by the Makefile with -ldflags "-X main.version=...". @@ -71,6 +72,19 @@ func run(args []string, stdout, stderr io.Writer) int { return cmd(g, rest[1:], stdout, stderr) } } + // Ruling 2026-09-12/4: Go's flag package stops parsing at the first + // non-flag argument, so "krino dl -n" leaves "-n" in rest as a second + // directory name instead of a flag - the dry run is silently never + // honoured. A leftover argument that still looks like a flag is a + // usage error rather than a guess; there is deliberately no second + // pass that re-parses trailing flags, which would make "krino -- + // -weird-dir" ambiguous between the two syntaxes. + for _, a := range rest { + if strings.HasPrefix(a, "-") { + fmt.Fprintf(stderr, "krino: %s: flags must come before directory names\nrun 'krino -h' for help\n", a) + return 2 + } + } return cmdSort(g, rest, stdout, stderr) } diff --git a/cmd/krino/main_test.go b/cmd/krino/main_test.go index 46ceb44..1c01fa0 100644 --- a/cmd/krino/main_test.go +++ b/cmd/krino/main_test.go @@ -51,10 +51,14 @@ func TestCommandsAreReserved(t *testing.T) { } } -func TestSortNotYet(t *testing.T) { +// TestSortNoConfig: apply is implemented as of Task 7, so running with no +// config at all now fails the same way every other command does — "not +// found" from engine.Load, not the old "not implemented yet" hard stop this +// test used to pin (removed as part of Task 7; see cmd/krino/sort.go). +func TestSortNoConfig(t *testing.T) { home(t) code, _, errOut := runCLI(t) - if code != 2 || !strings.Contains(errOut, "not implemented yet") { + if code != 2 || !strings.Contains(errOut, "not found; create it with: krino init") { t.Fatalf("got %d %q", code, errOut) } } diff --git a/cmd/krino/matching_test.go b/cmd/krino/matching_test.go index 614edc2..69c0eca 100644 --- a/cmd/krino/matching_test.go +++ b/cmd/krino/matching_test.go @@ -5,6 +5,7 @@ package main import ( "bytes" "encoding/json" + "fmt" "os" "path/filepath" "strings" @@ -148,8 +149,8 @@ func TestSortFlags(t *testing.T) { want string }{ {[]string{"-y", "-n"}, "-y and -n cannot be used together"}, - {nil, "applying files is not implemented yet; use -n to see what would happen"}, - {[]string{"--json"}, "--json is not implemented yet"}, // --json without -n stays an error + {nil, "not a terminal"}, // no -y, no -n, and the test's stdin is not one + {[]string{"--json"}, "--json is only valid with -n"}, // --json without -n stays an error } for _, tt := range tests { if code, _, errOut := runCLI(t, tt.args...); code != 2 || !strings.Contains(errOut, tt.want) { @@ -356,6 +357,100 @@ func TestDirectoryWarningNotCountedInWarningsField(t *testing.T) { // not stretched further), while a short name alongside it is still padded // out to the full 40-column cap — the layout stays a clean two-column grid // even though one row's first cell overruns it. +// TestApplyWithYesMovesFiles is brief 7's basic apply-path test: -y applies +// the plan with no prompt, the file actually moves, the outcome line says +// so, and the run is logged with both boundaries. +func TestApplyWithYesMovesFiles(t *testing.T) { + h := matchingFixture(t) + code, out, errOut := runCLI(t, "-y") + if code != 0 { + t.Fatalf("exit %d: %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, "dl", "Work", "Acme", "inv1.txt")); err != nil { + t.Errorf("inv1.txt was not filed: %v", err) + } + if !strings.Contains(out, "applied") { + t.Errorf("no outcome line:\n%s", out) + } + log := filepath.Join(h, ".local", "state", "krino", "krino.log") + b, err := os.ReadFile(log) + if err != nil { + t.Fatalf("nothing was logged: %v", err) + } + if !strings.Contains(string(b), "run-start") || !strings.Contains(string(b), "run-end") { + t.Errorf("log lacks run boundaries:\n%s", b) + } +} + +// TestDryRunLogsNothing is Ruling 2: journal.Open must never be called at +// all in dry-run mode, since it materialises both the state directory and +// an empty log file as a side effect of merely opening it. +func TestDryRunLogsNothing(t *testing.T) { + h := matchingFixture(t) + if code, _, errOut := runCLI(t, "-n"); code != 0 { + t.Fatalf("exit %d: %s", code, errOut) + } + if _, err := os.Stat(filepath.Join(h, ".local", "state", "krino", "krino.log")); !os.IsNotExist(err) { + t.Error("a dry run wrote to the log") + } +} + +// TestRefusesWithoutTerminalAndWithoutFlags is spec §8.4: with neither -y +// nor -n, and stdin not a terminal (as in every test), krino refuses rather +// than guess. +func TestRefusesWithoutTerminalAndWithoutFlags(t *testing.T) { + matchingFixture(t) + code, _, errOut := runCLI(t) + if code != 2 || !strings.Contains(errOut, "not a terminal") { + t.Errorf("no-flags, no-terminal: %d %q", code, errOut) + } +} + +// TestSecondRunFailsImmediatelyWithYes is spec §11: a second krino on the +// same directory fails immediately with -y (wait = false) rather than +// piling up behind a stuck run, so a cron job never queues silently. +func TestSecondRunFailsImmediatelyWithYes(t *testing.T) { + h := matchingFixture(t) + held := filepath.Join(h, ".local", "state", "krino", "dl.lock") + if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil { + t.Fatal(err) + } + // A lock held by this very process, so it is not stale. + if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil { + t.Fatal(err) + } + code, _, errOut := runCLI(t, "-y") + if code != 1 || !strings.Contains(errOut, "another krino") { + t.Errorf("-y against a held lock: %d %q", code, errOut) + } +} + +// TestFlagsMustPrecedeDirectoryNames is Ruling 4: Go's flag package stops +// parsing at the first non-flag argument, so "krino dl -n" would otherwise +// silently take "-n" as a second directory name and never honour the dry +// run. A leftover argument starting with "-" is a usage error instead of a +// guess. +func TestFlagsMustPrecedeDirectoryNames(t *testing.T) { + home(t) + code, _, errOut := runCLI(t, "dl", "-n") + if code != 2 || !strings.Contains(errOut, "-n") { + t.Errorf("krino dl -n: %d %q", code, errOut) + } +} + +// TestNoColourEscapeToNonTerminal is Ruling 7's one pinned guarantee: a plan +// piped to a file or read by another tool must be plain text. tui.Colour(w) +// already returns false for anything that is not a terminal *os.File, and +// runCLI's stdout is a bytes.Buffer, so this holds end to end through the +// real command path, not just at the helper that decides it. +func TestNoColourEscapeToNonTerminal(t *testing.T) { + matchingFixture(t) + _, out, _ := runCLI(t, "-n") + if strings.ContainsRune(out, '\x1b') { + t.Errorf("escape byte reached a non-terminal writer:\n%q", out) + } +} + func TestLongNameNotPaddedLayoutIntact(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") diff --git a/cmd/krino/render.go b/cmd/krino/render.go index b27d4d1..de5489a 100644 --- a/cmd/krino/render.go +++ b/cmd/krino/render.go @@ -77,6 +77,25 @@ func printPlan(w io.Writer, dp *engine.DirPlan, verbose bool) { } } +// chainActing reports whether c has at least one step that will actually +// run - the single definition of "actionable" that countActing, +// actionableChains (sort.go) and chainOutcomes (sort.go) all share (fix +// wave item 4 / Minor 5). Before this fix, countActing and actionableChains +// each kept their own copy of this question and disagreed: countActing +// excluded an all-skipped chain (len(Steps) > 0, but every step's Skip is +// set) while actionableChains's own len(Steps) > 0 check included it, so a +// directory could print "N scanned · 0 to act on" and then still ask the +// user to approve a file it had just said there were none of - and on +// approval, log a run-start/run-end pair holding only "skipped" entries. +func chainActing(c plan.Chain) bool { + for _, s := range c.Steps { + if s.Skip == "" { + return true + } + } + return false +} + // countActing reports how many chains have at least one step that will // actually run. C1/ruling 2026-09-12: a rule with no actions is an // exclusion, and a chain every one of whose steps is skipped is not about @@ -84,11 +103,8 @@ func printPlan(w io.Writer, dp *engine.DirPlan, verbose bool) { func countActing(chains []plan.Chain) int { n := 0 for _, c := range chains { - for _, s := range c.Steps { - if s.Skip == "" { - n++ - break - } + if chainActing(c) { + n++ } } return n diff --git a/cmd/krino/review.go b/cmd/krino/review.go new file mode 100644 index 0000000..4f5ad05 --- /dev/null +++ b/cmd/krino/review.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "fmt" + "io" + "os" + "strings" + + "krino/internal/plan" + "krino/internal/tui" +) + +// reviewDir drives spec §8.2/§8.3's interactive review over the real +// terminal. It is a thin wrapper: reviewChains holds all the actual +// approval logic, driven here by tui.ReadKey (raw mode while stdin is a +// terminal, a plain single-byte read otherwise) via keyReader, so the exact +// same code path runs whether the input is a real keypress or, in tests, a +// strings.Reader. root is the directory being reviewed, threaded through to +// reviewChains so a per-file destination renders the same way the +// directory-level table does (root-relative inside root, ~-abbreviated +// outside it) instead of always falling back to the abbreviated form. +func reviewDir(out io.Writer, chains []plan.Chain, root string) (map[string]bool, rune, error) { + return reviewChains(keyReader{stdin}, out, chains, root) +} + +// keyReader adapts tui.ReadKey - one key at a time, from a real *os.File - +// to the io.Reader reviewChains expects. tui.ReadKey's own doc comment is +// why this is safe to call once per key: it always reads exactly one byte +// and restores the terminal on every path before returning. +type keyReader struct{ f *os.File } + +func (k keyReader) Read(p []byte) (int, error) { + r, err := tui.ReadKey(k.f) + if err != nil { + return 0, err + } + p[0] = byte(r) + return 1, nil +} + +// reviewChains is spec §8.2/§8.3's approval flow, and the testable core +// reviewDir wraps: the top-level +// +// [a] apply all [c] choose per file [s] skip this directory [q] quit +// +// menu, and, for [c], the per-file +// +// [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing +// +// prompt. action is always one of 'a', 'c', 's' or 'q': a [c] session's own +// [q] ("quit, apply nothing") folds into the same 'q' the caller already +// handles for the top-level menu, and approved is emptied to match - even a +// file already marked yes in that session is discarded, per spec §8.3's +// wording ("apply nothing"), unlike [d] ("apply chosen so far"), which +// keeps it. root is the directory being reviewed - passed only to +// reviewPerFile's destination rendering (review finding 1, fix round +// 2026-09-12); nothing here uses it directly. +func reviewChains(in io.Reader, out io.Writer, chains []plan.Chain, root string) (map[string]bool, rune, error) { + fmt.Fprint(out, "\n[a] apply all [c] choose per file [s] skip this directory [q] quit\n") + for { + key, err := readKey(in) + if err != nil { + return nil, 0, err + } + switch key { + case 'a': + return approveAll(chains), 'a', nil + case 's': + return map[string]bool{}, 's', nil + case 'q': + return map[string]bool{}, 'q', nil + case 'c': + approved, quit, err := reviewPerFile(in, out, chains, root) + if err != nil { + return nil, 0, err + } + if quit { + return map[string]bool{}, 'q', nil + } + return approved, 'c', nil + default: + fmt.Fprintf(out, "%q is not a, c, s or q\n", key) + } + } +} + +// reviewPerFile is spec §8.3: one prompt per file, in the order chains +// already carries them (the same order the numbered table above it was +// shown in, per D15 in render.go). [y]/[n] decide just that file; [a] +// approves it and every remaining file without asking again; [d] stops +// asking and applies whatever was already chosen, declining the rest; [q] +// aborts the review entirely, discarding even files already marked yes - +// reported back to reviewChains via quit=true. root is passed to +// actionCell exactly as the directory-level table (render.go's planRows) +// already does, so a destination inside root renders root-relative and one +// outside it renders ~-abbreviated - review finding 1 (fix round +// 2026-09-12): passing "" here always fails filepath.Rel("", dir) and +// silently fell back to the abbreviated form even for a destination inside +// root, which is not what spec §8.3's own worked example shows. +func reviewPerFile(in io.Reader, out io.Writer, chains []plan.Chain, root string) (approved map[string]bool, quit bool, err error) { + approved = map[string]bool{} + yesRest := false + for i, c := range chains { + if yesRest { + approved[c.File.Rel] = true + continue + } + + fmt.Fprintf(out, "\n[%d/%d] %s\n", i+1, len(chains), c.File.Rel) + for _, s := range c.Steps { + fmt.Fprintf(out, " %s\n", actionCell(s, root)) + } + fmt.Fprint(out, " [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing\n") + + for { + key, kerr := readKey(in) + if kerr != nil { + return nil, false, kerr + } + switch key { + case 'y': + approved[c.File.Rel] = true + case 'n': + // leave unapproved + case 'a': + approved[c.File.Rel] = true + yesRest = true + case 'd': + return approved, false, nil + case 'q': + return nil, true, nil + default: + fmt.Fprintf(out, "%q is not y, n, a, d or q\n", key) + continue + } + break + } + } + return approved, false, nil +} + +// readKey reads the single byte reviewChains treats as one keypress. Over +// the real terminal that byte already came from tui.ReadKey (via +// keyReader); in a test, it is just the next byte of a strings.Reader. +func readKey(in io.Reader) (rune, error) { + var b [1]byte + if _, err := io.ReadFull(in, b[:]); err != nil { + return 0, err + } + return rune(b[0]), nil +} + +// approveAll approves every one of chains by File.Rel - [a] apply all at +// the top level, and [a] yes to this and all remaining once it fires +// mid per-file review. +func approveAll(chains []plan.Chain) map[string]bool { + approved := make(map[string]bool, len(chains)) + for _, c := range chains { + approved[c.File.Rel] = true + } + return approved +} + +// colourDeletePermanently wraps spec §8.2's "DELETE permanently" marker in +// the terminal's own ANSI red (bold, slot 1 - never hex), the one thing the +// spec singles out for emphasis (Ruling 2026-09-12/7). It is applied to +// text render.go's printPlan already produced, rather than threading a +// colour parameter through the renderer itself: that keeps render.go and +// its golden-file tests exactly as plan 4 built them. colour is always the +// caller's own tui.Colour(stdout) decision - with it false this is a no-op, +// which is what keeps every escape byte out of a plan piped to a file or +// read by another tool. +func colourDeletePermanently(text string, colour bool) string { + if !colour { + return text + } + return strings.ReplaceAll(text, "DELETE permanently", "\x1b[1;31mDELETE permanently\x1b[0m") +} diff --git a/cmd/krino/review_test.go b/cmd/krino/review_test.go new file mode 100644 index 0000000..193a191 --- /dev/null +++ b/cmd/krino/review_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "strings" + "testing" + + "krino/internal/plan" + "krino/internal/scan" +) + +func chains(rels ...string) []plan.Chain { + out := make([]plan.Chain, len(rels)) + for i, r := range rels { + out[i] = plan.Chain{File: scan.File{Rel: r}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/w/" + r}}} + } + return out +} + +func TestChoosePerFile(t *testing.T) { + // c enters per-file mode, then y n y for three files. + approved, action, err := reviewChains(strings.NewReader("cyny"), new(strings.Builder), chains("a", "b", "c"), "") + if err != nil { + t.Fatal(err) + } + if action != 'c' { + t.Errorf("action = %q", action) + } + if !approved["a"] || approved["b"] || !approved["c"] { + t.Errorf("approved = %v; want a and c only", approved) + } +} + +func TestApplyAllAndSkip(t *testing.T) { + approved, action, _ := reviewChains(strings.NewReader("a"), new(strings.Builder), chains("a", "b"), "") + if action != 'a' || len(approved) != 2 { + t.Errorf("[a] = %q %v; want every file approved", action, approved) + } + approved, action, _ = reviewChains(strings.NewReader("s"), new(strings.Builder), chains("a", "b"), "") + if action != 's' || len(approved) != 0 { + t.Errorf("[s] = %q %v; want nothing approved", action, approved) + } +} + +func TestPerFileDoneStopsAsking(t *testing.T) { + // c, y for the first, then d: apply what was chosen so far. + approved, _, _ := reviewChains(strings.NewReader("cyd"), new(strings.Builder), chains("a", "b", "c"), "") + if !approved["a"] || approved["b"] || approved["c"] { + t.Errorf("approved = %v; want only a", approved) + } +} + +// TestPerFileQuitAppliesNothing: spec §8.3's [q] on the per-file prompt is +// "quit, apply nothing" - stronger than [d], which keeps what was already +// chosen. It folds into the same top-level 'q' the caller already handles +// for the directory-level menu (spec §8.2's [q]), and discards even a file +// already marked yes. +func TestPerFileQuitAppliesNothing(t *testing.T) { + approved, action, err := reviewChains(strings.NewReader("cyq"), new(strings.Builder), chains("a", "b"), "") + if err != nil { + t.Fatal(err) + } + if action != 'q' { + t.Errorf("action = %q, want 'q'", action) + } + if len(approved) != 0 { + t.Errorf("approved = %v; want nothing, even though a was marked yes first", approved) + } +} + +// TestPerFileYesToAllRemaining: spec §8.3's [a] mid-review approves the +// current file and every remaining one without asking again. +func TestPerFileYesToAllRemaining(t *testing.T) { + approved, action, err := reviewChains(strings.NewReader("ca"), new(strings.Builder), chains("a", "b", "c"), "") + if err != nil { + t.Fatal(err) + } + if action != 'c' || len(approved) != 3 { + t.Errorf("approved = %v action = %q; want all three approved", approved, action) + } +} + +// TestInvalidKeyReprompts: an unrecognised key at either the top-level menu +// or the per-file prompt does not abort the review - it is reported and the +// same prompt is read again. +func TestInvalidKeyReprompts(t *testing.T) { + out := new(strings.Builder) + approved, action, err := reviewChains(strings.NewReader("zs"), out, chains("a"), "") + if err != nil { + t.Fatal(err) + } + if action != 's' || len(approved) != 0 { + t.Errorf("approved = %v action = %q; want [s] after the bad key", approved, action) + } + if !strings.Contains(out.String(), "z") { + t.Errorf("no mention of the rejected key:\n%s", out) + } +} + +// TestPerFileDestinationIsRootRelative is review finding 1 (fix round +// 2026-09-12): reviewPerFile must render a destination the same way the +// directory-level table does (render.go's destText) - root-relative for a +// destination inside root, ~-abbreviated for one outside it - not always +// abbreviated because root was never passed through to actionCell at all. +// Both halves are pinned: getting only the "inside" half right would still +// let an outside-root destination silently regress to some other form. +func TestPerFileDestinationIsRootRelative(t *testing.T) { + t.Setenv("HOME", "/home/x") + root := "/home/x/dl" + cs := []plan.Chain{ + {File: scan.File{Rel: "inside.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: root + "/Work/Acme/inside.txt"}}}, + {File: scan.File{Rel: "outside.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/home/x/backup/outside.txt"}}}, + } + out := new(strings.Builder) + if _, _, err := reviewChains(strings.NewReader("cyy"), out, cs, root); err != nil { + t.Fatal(err) + } + text := out.String() + if !strings.Contains(text, "move → Work/Acme/") { + t.Errorf("destination inside root should be root-relative, not ~-abbreviated:\n%s", text) + } + if !strings.Contains(text, "move → ~/backup/") { + t.Errorf("destination outside root should be ~-abbreviated:\n%s", text) + } +} + +// TestNoColourEscapeFromColourFalse pins Ruling 7's safety guarantee at the +// unit that actually decides it: with colour off, colourDeletePermanently +// must not alter the text at all, and with colour on it must add an escape +// around exactly the one marker spec §8.2 singles out for emphasis. +func TestColourDeletePermanently(t *testing.T) { + plain := " 4 setup-1.2.deb DELETE permanently old-pkgs age 94d\n" + if got := colourDeletePermanently(plain, false); got != plain { + t.Errorf("colour=false must leave the text untouched:\n%q", got) + } + got := colourDeletePermanently(plain, true) + if !strings.Contains(got, "\x1b[") { + t.Errorf("colour=true should add an escape sequence:\n%q", got) + } + if !strings.Contains(got, "DELETE permanently") { + t.Errorf("colour=true should not remove the marker text itself:\n%q", got) + } +} diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go index 0ba82bc..68f9eda 100644 --- a/cmd/krino/sort.go +++ b/cmd/krino/sort.go @@ -3,34 +3,59 @@ package main import ( + "bytes" "context" "encoding/json" + "errors" "fmt" "io" "os" + "os/signal" "sort" "strings" + "syscall" "unicode/utf8" + "golang.org/x/term" + "krino/internal/engine" + "krino/internal/journal" + "krino/internal/lock" "krino/internal/plan" "krino/internal/scan" + "krino/internal/tui" "krino/internal/xdg" ) -// cmdSort plans and applies the included directories. Only -n (dry run) is -// implemented; applying arrives in plan 4. +// stdin is os.Stdin, threaded through this seam rather than referenced +// directly: cmdSort's terminal check, installSignalHandler and reviewDir +// all read it, and a test must never depend on what the ambient test +// binary's stdin happens to be (fix round 2026-09-12/item 4). If it were +// ever a real terminal, code that only worked by assuming otherwise would +// fall through to the interactive prompt and block the test suite on a +// keypress - the same kind of hang the lock-cancellation test was built to +// never risk. Tests point this at something guaranteed non-terminal +// (commands_test.go's home helper) instead of relying on a claim about +// what go test does with stdin. +var stdin = os.Stdin + +// zeroOutcome is the per-directory outcome line for a directory that had +// nothing applied to it - either because nothing was actionable, or +// because the user chose [s] or [q] - so the same wording is not retyped +// (and cannot drift) across the three places it applies. +const zeroOutcome = "0 applied · 0 failed · 0 declined" + +// cmdSort plans and, from Task 7, applies the included directories: flags +// are checked before any config is read, a bad config stops the whole run +// before scanning (spec §11), and one journal.Writer, run id and +// plan.Claims cover every directory in the run. See docs/design.md +// §8.2-§8.4 and §11 for the flow this follows. func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { if g.yes && g.dry { return usageError(stderr, "-y and -n cannot be used together") } if g.json && !g.dry { - fmt.Fprintln(stderr, "krino: --json is not implemented yet") - return 2 - } - if !g.dry { - fmt.Fprintln(stderr, "krino: applying files is not implemented yet; use -n to see what would happen") - return 2 + return usageError(stderr, "--json is only valid with -n") } e, errs := engine.Load(mainFile(g), names...) @@ -39,6 +64,50 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { return 2 } + // Spec §8.4: with neither -y nor -n, krino asks; asking a non-terminal + // stdin would just hang (or read garbage), so it refuses instead. + if !g.yes && !g.dry && !term.IsTerminal(int(stdin.Fd())) { + return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)") + } + + // Ruling 5: internal/tui deliberately does not trap signals - a package + // that installs process-wide handlers as a side effect of reading one + // key would surprise every caller. It lands here because cmdSort must + // install one anyway: spec §11 says Ctrl-C finishes the current step, + // logs it, and stops, which means the ctx passed to Apply below must be + // cancelled on SIGINT. The same handler also restores the terminal on + // SIGTERM, which - unlike a keyboard Ctrl-C during tui.ReadKey's raw + // read (ISIG is off, so that never even reaches us as a signal) - can + // land mid-read with no defer left to run. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stopSignals := installSignalHandler(cancel) + defer stopSignals() + + // Ruling 2: journal.Open creates $XDG_STATE_HOME/krino/ and an empty + // krino.log as a side effect of merely being called, so a dry run must + // never call it at all - not open it and clean up afterwards. -n is + // known from the flags before the loop starts, so the gate is exactly + // that, nothing per-directory. One Writer and one run id cover every + // directory in the run (Task 5's undo depends on a single run id + // spanning all of them). + var j *journal.Writer + var run string + if !g.dry { + var err error + j, err = journal.Open(e.Config.LogFile()) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + if cerr := j.Close(); cerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", cerr) + } + }() + run = journal.NewRunID(e.Now()) + } + exit := 0 printed := false jsonDirs := []plan.JSONDir{} // never nil: the document's "dirs" must marshal as [], not null @@ -47,36 +116,154 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { // same destination resolve the collision at planning time instead of // each independently believing it owns that path. claims := plan.NewClaims() + for _, d := range e.Dirs { - if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() { - fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root)) - exit = 1 - continue - } - dp, err := e.Plan(context.Background(), d, claims) + // Spec §3/§11: a second krino on the same directory waits for the + // lock, or fails immediately with -y, so a cron job never piles up + // behind a stuck run. lock.Acquire takes ctx precisely so that wait + // is not unbounded in practice (fix round 2026-09-12/item 1): a + // signal cancels it and Acquire returns ctx.Err() promptly instead + // of polling forever. + l, err := lock.Acquire(ctx, e.Config.LockFile(d.Name), !g.yes) if err != nil { - fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Interrupted while waiting for the lock: an interrupt, not + // a failure - the ctx.Err() check at the end of this + // function already turns this into exit 130, and nothing + // further should even be attempted. + break + } + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err) exit = 1 continue } - if !g.json { - if printed { - fmt.Fprintln(stdout) + + quit := func() bool { + defer func() { + if rerr := l.Release(); rerr != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr) + } + }() + + if fi, err := os.Stat(d.Root); err != nil || !fi.IsDir() { + fmt.Fprintf(stderr, "krino: skipping %s: %s is not a directory\n", d.Name, xdg.Abbrev(d.Root)) + exit = 1 + return false } - printed = true - fmt.Fprintf(stdout, "krino: %s %s\n", d.Name, xdg.Abbrev(d.Root)) - } - // C3: directory-level warnings go to stderr after the header - // line above, not before it, so on a terminal they read as - // describing the directory just named instead of floating above it. - for _, w := range dp.Result.Warnings { - fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w) - } - if g.json { - jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, dp.Result.Warnings)) - continue + dp, err := e.Plan(ctx, d, claims) + if err != nil { + fmt.Fprintf(stderr, "krino: skipping %s: %v\n", d.Name, err) + exit = 1 + return false + } + + if !g.json { + if printed { + fmt.Fprintln(stdout) + } + printed = true + fmt.Fprintf(stdout, "krino: %s %s\n", d.Name, xdg.Abbrev(d.Root)) + } + // C3: directory-level warnings go to stderr after the header + // line above, not before it, so on a terminal they read as + // describing the directory just named instead of floating above it. + for _, w := range dp.Result.Warnings { + fmt.Fprintf(stderr, "krino: %s: %s\n", d.Name, w) + } + if g.json { + // --json is only ever reached with -n (checked above), and + // Ruling 7 is explicit that JSON must never be paged, so + // this returns before any of the paging/review code below. + jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, dp.Result.Warnings)) + return false + } + + // Ruling 6: the plan goes through tui.Page - taller than the + // terminal, it is shown through $PAGER and the prompt follows + // once the pager exits (spec §8.2) - for -n as much as for the + // interactive and -y paths; a dry run that scrolls 200 files + // off the top of the terminal is exactly the case the pager + // exists for. printPlan is reused as-is (render.go), never + // re-rendered here. + var buf bytes.Buffer + printPlan(&buf, dp, g.verbose) + text := colourDeletePermanently(buf.String(), tui.Colour(stdout)) + if err := tui.Page(stdout, text); err != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, err) + exit = 1 + return false + } + + if g.dry { + return false + } + + actionable := actionableChains(dp.Chains) + if len(actionable) == 0 { + // Nothing to decide: neither -y nor the interactive menu + // has anything useful to do here, so neither is asked. + fmt.Fprintln(stdout, zeroOutcome) + return false + } + + var approved map[string]bool + var action rune + if g.yes { + approved, action = approveAll(actionable), 'a' + } else { + var rerr error + approved, action, rerr = reviewDir(stdout, actionable, d.Root) + if rerr != nil { + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, rerr) + exit = 1 + return false + } + } + + switch action { + case 's': + // Spec §8.2: [s] applies nothing in this directory and + // moves on - no Apply call at all, so nothing is logged + // for it either (Ruling 1: this is a chosen outcome, not a + // failure, and must not set exit 1). + fmt.Fprintln(stdout, zeroOutcome) + return false + case 'q': + // Spec §8.2: [q] stops krino; directories already applied + // this run stay applied. Nothing is applied here either, + // and no further directory is even planned. + fmt.Fprintln(stdout, zeroOutcome) + return true + } + + res, aerr := e.Apply(ctx, dp, approved, j, run) + if aerr != nil { + if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) { + // Interrupted mid-apply (fix round 2026-09-12/item 2): + // treated exactly like the cancelled lock wait above - + // not a failure ("context canceled" is a Go-ism, not + // something to show a user who just pressed Ctrl-C), + // and no further directory is even attempted. The + // ctx.Err() check at the end of this function already + // turns this into exit 130. + return true + } + fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, aerr) + exit = 1 + return false + } + fmt.Fprintf(stdout, "%d applied · %d failed · %d declined\n", res.Applied, res.Failed, res.Declined) + // Ruling 1: only an actual step failure makes the run exit 1 + // here - a directory the user declined or skipped must not. + if res.Failed > 0 { + exit = 1 + } + return false + }() + + if quit { + break } - printPlan(stdout, dp, g.verbose) } if g.json { @@ -90,9 +277,107 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int { stdout.Write(b) fmt.Fprintln(stdout) } + + // Spec §11: 130 interrupted takes priority over whatever exit already + // accumulated - ctx is only ever cancelled by installSignalHandler, and + // package main's own cancel() (deferred above) has not run yet here. + if ctx.Err() != nil { + return 130 + } return exit } +// actionableChains returns the chains of dp.Chains that have at least one +// step that will actually run (chainActing, render.go) - fix wave item 4 / +// Minor 5: this used to be a separate len(c.Steps) > 0 check, which +// disagreed with render.go's countActing over a chain every one of whose +// steps is skipped, so a directory could report "0 to act on" and then +// still offer such a chain for approval. Converged on chainActing, this is +// now also stricter than the filter engine.Apply's own forward-path loop +// applies (internal/engine/apply.go's Apply, still len(c.Steps) > 0): an +// all-skipped chain is simply never a candidate for approval here, so it +// can never reach Apply with approved == true, and Apply's own loop - +// unchanged - logs it as declined exactly like any other file this review +// never approved (tallyFile then counts it there, not as a fall-through). +func actionableChains(chains []plan.Chain) []plan.Chain { + var out []plan.Chain + for _, c := range chains { + if chainActing(c) { + out = append(out, c) + } + } + return out +} + +// installSignalHandler arranges for SIGINT and SIGTERM to cancel cancel +// and, if stdin is a terminal, restore it to the state it was in when this +// was called (Ruling 5). The returned func stops the handler and must be +// called once the run is over, or its goroutine and signal registration +// outlive cmdSort. +// +// Fix round 2026-09-12/item 2: the handler loops rather than servicing one +// signal and exiting. A single-shot select left signal.Notify's +// registration in place (which suppresses Go's default terminate) with no +// goroutine left reading the channel, so a second Ctrl-C landed in the +// buffered channel unread and a third was dropped outright - together with +// lock.Acquire's own fix, that made a run waiting on a held lock ignore +// every Ctrl-C and every SIGTERM forever, with no escape but another +// shell's kill -9. Looping fixes the common case (ctx cancellation reaches +// something that is actually checking it, e.g. a waiting lock.Acquire or +// Apply between files) and the second signal is also the user's guarantee +// of an exit even when it does not: by then a clean shutdown has already +// been asked for once and not delivered, so it restores the terminal once +// more (harmless if already restored) and exits immediately with the same +// 130 spec §11 already uses for "interrupted". +func installSignalHandler(cancel context.CancelFunc) func() { + fd := int(stdin.Fd()) + var saved *term.State + if term.IsTerminal(fd) { + saved, _ = term.GetState(fd) // best-effort: nothing to restore if this fails + } + + sig := make(chan os.Signal, 1) + signal.Notify(sig, os.Interrupt, syscall.SIGTERM) + done := make(chan struct{}) + go func() { + signals := 0 + for { + select { + case <-sig: + cancel() + if saved != nil { + term.Restore(fd, saved) + } + signals++ + if signals >= 2 { + // Deliberate exception to "the lock is released on + // every path" (fix round 2026-09-12/item 3, by + // design, documented on review): every deferred + // cleanup in cmdSort - including the held directory's + // lock.Release - is skipped here. That is intentional: + // this is the user's escape hatch when a clean + // shutdown was already asked for once (the first + // signal) and not delivered, so trying to unwind + // cleanly a second time is exactly what would make the + // hatch unreliable. It is safe to skip that unwind: + // journal.Append flushes each line as it writes, so + // nothing buffered is lost by exiting immediately, and + // an abandoned lock file is reclaimed automatically by + // Task 4's stale-pid takeover the next time anything + // tries to acquire it (lock.go's tryAcquire). + os.Exit(130) + } + case <-done: + return + } + } + }() + return func() { + signal.Stop(sig) + close(done) + } +} + // warnLine is one file's warning, for the warnings section. type warnLine struct { rel string @@ -235,14 +520,7 @@ func chainOutcomes(chains []plan.Chain) (excluded, allSkipped int) { excluded++ continue } - acting := false - for _, s := range c.Steps { - if s.Skip == "" { - acting = true - break - } - } - if !acting { + if !chainActing(c) { allSkipped++ } } diff --git a/cmd/krino/sort_test.go b/cmd/krino/sort_test.go index a646503..e482f44 100644 --- a/cmd/krino/sort_test.go +++ b/cmd/krino/sort_test.go @@ -2,7 +2,16 @@ package main -import "testing" +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "krino/internal/plan" + "krino/internal/scan" +) // TestRelWidthAndPadCellCountRunes: C4. relWidth and padCell must measure // column width in runes, not bytes, or a name carrying diacritics @@ -21,3 +30,83 @@ func TestRelWidthAndPadCellCountRunes(t *testing.T) { t.Fatalf("padCell(%q, 9) = %q, want %q (already at width: no padding)", "próba.txt", got, want) } } + +// TestActionableChainsAgreesWithCountActing is fix wave item 4 / Minor 5: +// countActing (render.go) and actionableChains used to disagree over a +// chain every one of whose steps is skipped (len(Steps) > 0, but every +// step's own Skip is set) - countActing already excluded it from "to act +// on", while actionableChains's own len(Steps) > 0 check still offered it +// for approval, so a directory could print "N scanned · 0 to act on" and +// then still ask the user to approve a file it had just said there were +// none of. Converged on chainActing (render.go), both must now agree. +func TestActionableChainsAgreesWithCountActing(t *testing.T) { + chains := []plan.Chain{ + {File: scan.File{Rel: "a.txt"}, Steps: []plan.Step{{Kind: plan.Move, Skip: "target exists"}}}, + {File: scan.File{Rel: "b.txt"}, Steps: []plan.Step{{Kind: plan.Move, Dst: "/r/W/b.txt"}}}, + } + if got := countActing(chains); got != 1 { + t.Errorf("countActing = %d, want 1 (a.txt is all-skipped)", got) + } + actionable := actionableChains(chains) + if len(actionable) != 1 || actionable[0].File.Rel != "b.txt" { + t.Errorf("actionableChains = %+v, want only b.txt - an all-skipped chain must never be offered for approval", actionable) + } +} + +// TestAllSkippedDirectoryReportsZeroAndLogsNothing is fix wave item 4 / +// Minor 5 and 6, end to end. Before the fix: a directory whose one file +// matches a rule under (on-conflict skip) - so its single step's own Skip +// is set ("target exists") - printed "0 to act on" (countActing) and then, +// with -y, still ran that chain through Apply anyway (actionableChains' +// own len(Steps) > 0 check approved it regardless), logging a +// run-start/run-end pair holding only a "skipped" entry while the outcome +// line read "0 applied · 0 failed · 0 declined" for a file that had just +// been silently processed. After the fix, the chain is never offered for +// approval, Apply is never even called for this directory, and the journal +// gains nothing at all. +func TestAllSkippedDirectoryReportsZeroAndLogsNothing(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(filepath.Join(dl, "Out"), 0o755); err != nil { + t.Fatal(err) + } + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for _, p := range []string{filepath.Join(dl, "a.txt"), filepath.Join(dl, "Out", "a.txt")} { + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + 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(min-age 0s)\n(on-conflict skip)\n(rule \"r\" (when (type text)) (move \"Out\"))\n" + if err := os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + + code, out, errOut := runCLI(t, "-y") + if code != 0 { + t.Fatalf("run: %d %s", code, errOut) + } + if !strings.Contains(out, "1 scanned · 0 to act on") { + t.Errorf("output = %q, want \"0 to act on\"", out) + } + if !strings.Contains(out, zeroOutcome) { + t.Errorf("output = %q, want the honest zero outcome %q", out, zeroOutcome) + } + + logPath := filepath.Join(h, ".local", "state", "krino", "krino.log") + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("reading the journal: %v", err) + } + if len(data) != 0 { + t.Errorf("journal gained entries for a directory with nothing to act on:\n%s", data) + } +} diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go new file mode 100644 index 0000000..7ffe428 --- /dev/null +++ b/cmd/krino/undo.go @@ -0,0 +1,533 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "sort" + "strconv" + "strings" + + "golang.org/x/term" + + "krino/internal/config" + "krino/internal/engine" + "krino/internal/journal" + "krino/internal/lock" + "krino/internal/tui" + "krino/internal/xdg" +) + +func init() { commands["undo"] = cmdUndo } + +// cmdUndo reverses a run: the one named on the command line, or (spec §10) +// the most recent one otherwise. Since the newest run in the log can never +// itself be marked Undone - that would require a still-later run to have +// reversed it - "the most recent run" and "the most recent run that has not +// been undone" are the same run in every case, including the one this +// task's own test exercises: undoing an undo run a second time with no RUN +// argument targets that very undo run, which PlanUndo then refuses by name. +// +// Undo builds a plan like any other, shown and approved the same way (spec +// §10) - reviewUndoDir/-Files/-PerFile below are undo's own counterpart to +// review.go's reviewChains/reviewPerFile, not a call into them: an undo plan +// is []engine.UndoFile, which can span several directories in one flat +// list, so two files from different directories can share the same Rel and +// approval here is keyed by index rather than by name. +func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int { + fs := flagSet("undo", g) + fs.BoolVar(&g.yes, "y", false, "") + fs.BoolVar(&g.dry, "n", false, "") + if code, ok := parse(fs, args, stdout, stderr); !ok { + return code + } + if g.yes && g.dry { + return usageError(stderr, "-y and -n cannot be used together") + } + rest := fs.Args() + if len(rest) > 1 { + return usageError(stderr, "usage: krino undo [RUN]") + } + + e, errs := engine.Load(mainFile(g)) + if len(errs) > 0 { + printDiags(stderr, errs) + return 2 + } + + // Spec §8.4/§10: with neither -y nor -n, krino asks; a non-terminal + // stdin would just hang, so it refuses instead - the same check + // cmdSort makes before it ever shows a plan. + if !g.yes && !g.dry && !term.IsTerminal(int(stdin.Fd())) { + return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)") + } + + // Installed here, before any paging or review, not just around + // ApplyUndo: Ruling 5 (Task 7) is that SIGTERM landing between + // keystrokes or during the pager needs the terminal restored, and that + // window starts as soon as this command might show something on a + // terminal. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + stopSignals := installSignalHandler(cancel) + defer stopSignals() + + runID := "" + if len(rest) == 1 { + runID = rest[0] + } else { + runs, err := e.Runs(1) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + fmt.Fprintln(stdout, "nothing logged yet; nothing to undo") + return 0 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + if len(runs) == 0 { + fmt.Fprintln(stdout, "nothing logged yet; nothing to undo") + return 0 + } + runID = runs[0].ID + } + + // PlanUndo only reads the log; nothing is touched yet (spec §10), which + // is what makes it safe to call before any lock is taken. + up, err := e.PlanUndo(runID) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + + // Fix round 2026-09-12 (widened per the coordinator's follow-up + // ruling): an undo moves files just as an apply does, so it needs + // cmdSort's same per-directory guard (spec §11: a second krino on the + // same directory waits for the lock, or fails immediately with -y), + // held across the SAME window cmdSort holds its own lock across - the + // plan display and the review, not just the apply. Failing before the + // plan is even shown is strictly kinder than making the user review + // (potentially hundreds of files) only to be refused afterward, and it + // means a plan actually reviewed cannot go stale under the reader's + // eyes from another krino moving those same files mid-review. -n never + // reaches this: it changes nothing, so it takes no lock either. One + // undo run can span several directories (UndoFile.Dir is per file), so + // every distinct one up.Files touches is locked, in a fixed (sorted) + // order, and released on every path below - including [s]/[q], every + // early return, and a later ApplyUndo failure - by the single defer + // right after acquisition. + var locks []*lock.Lock + if !g.dry { + locks, err = acquireUndoLocks(ctx, e.Config, undoDirNames(up.Files), !g.yes) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return 130 + } + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + for _, rerr := range releaseUndoLocks(locks) { + fmt.Fprintf(stderr, "krino: %v\n", rerr) + } + }() + } + + fmt.Fprintf(stdout, "krino: undo %s\n", up.Run) + var buf bytes.Buffer + printUndoPlan(&buf, up) + text := colourRefused(buf.String(), tui.Colour(stdout)) + // Ruling 6 (Task 7), carried over: the plan goes through tui.Page for + // -n as much as for -y and the interactive path. + if err := tui.Page(stdout, text); err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + + if g.dry { + return 0 + } + + if undoActionableCount(up.Files) == 0 { + // Every file was refused, or the run touched none at all: neither + // -y nor the interactive menu has anything useful to do (mirrors + // cmdSort's identical check before it ever asks). + fmt.Fprintln(stdout, zeroOutcome) + return 0 + } + + var approved map[int]bool + var action rune + if g.yes { + approved, action = approveAllUndo(up.Files), 'a' + } else { + var rerr error + approved, action, rerr = reviewUndoDir(stdout, up.Files) + if rerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", rerr) + return 1 + } + } + + if action == 's' || action == 'q' { + fmt.Fprintln(stdout, zeroOutcome) + return 0 + } + + toApply := finalizeUndoPlan(up, approved) + + // Ruling 2 (Task 7), carried over: journal.Open creates the state + // directory and the log file as a side effect of merely being called, + // so it is opened only once we know something will actually be + // applied - never for -n (returned above), and not merely because -y + // or a review session ran, unlike cmdSort's own eager-open (which opens + // before it knows whether anything is actionable, a difference forced + // by cmdSort not yet having a plan to inspect at that point in its + // flow; undo already does, so it opens later, and never opens if + // undoActionableCount was 0 or the user chose [s]/[q] above). + j, err := journal.Open(e.Config.LogFile()) + if err != nil { + fmt.Fprintf(stderr, "krino: %v\n", err) + return 1 + } + defer func() { + if cerr := j.Close(); cerr != nil { + fmt.Fprintf(stderr, "krino: %v\n", cerr) + } + }() + run := journal.NewRunID(e.Now()) + + res, aerr := e.ApplyUndo(ctx, toApply, j, run) + if aerr != nil { + if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) { + // Interrupted mid-apply: the ctx.Err() check below turns this + // into exit 130, same as cmdSort. + return 130 + } + fmt.Fprintf(stderr, "krino: %v\n", aerr) + return 1 + } + fmt.Fprintf(stdout, "%d applied · %d failed · %d declined\n", res.Applied, res.Failed, res.Declined) + + if ctx.Err() != nil { + return 130 + } + if res.Failed > 0 { + return 1 + } + return 0 +} + +// undoActionableCount counts the files in files that PlanUndo has not +// already refused - the same "is there anything to even ask about" gate +// cmdSort's actionableChains serves for a sort plan. +func undoActionableCount(files []engine.UndoFile) int { + n := 0 + for _, f := range files { + if f.Refused == "" { + n++ + } + } + return n +} + +// undoDirNames returns the distinct directory names an undo plan's files +// touch, sorted: a fixed order so two processes each locking a plan that +// shares more than one directory always acquire them in the same sequence, +// the standard way to avoid a lock-order deadlock. UndoFile.Dir is the +// journal's own `dir` column - the directory's config NAME (e.g. "dl"), not +// a filesystem path - which is exactly what config.Config.LockFile takes. +func undoDirNames(files []engine.UndoFile) []string { + seen := map[string]bool{} + var out []string + for _, f := range files { + if f.Dir != "" && !seen[f.Dir] { + seen[f.Dir] = true + out = append(out, f.Dir) + } + } + sort.Strings(out) + return out +} + +// acquireUndoLocks takes the lock for every name in dirs, in order, +// mirroring cmdSort's per-directory lock.Acquire call. If any acquisition +// fails - held with wait false, or ctx cancelled while waiting - every lock +// already taken is released before returning, so a partial lock set is +// never left held while the caller reports the error and stops. +func acquireUndoLocks(ctx context.Context, cfg *config.Config, dirs []string, wait bool) ([]*lock.Lock, error) { + locks := make([]*lock.Lock, 0, len(dirs)) + for _, name := range dirs { + l, err := lock.Acquire(ctx, cfg.LockFile(name), wait) + if err != nil { + releaseUndoLocks(locks) + return nil, fmt.Errorf("%s: %w", name, err) + } + locks = append(locks, l) + } + return locks, nil +} + +// releaseUndoLocks releases every lock in locks and returns any release +// errors, one lock's failure never stopping the rest from being released - +// the same "release on every path" guarantee cmdSort gives its own single +// lock, extended to however many an undo plan needed. +func releaseUndoLocks(locks []*lock.Lock) []error { + var errs []error + for _, l := range locks { + if err := l.Release(); err != nil { + errs = append(errs, err) + } + } + return errs +} + +// finalizeUndoPlan builds the *engine.UndoPlan ApplyUndo actually runs, +// preserving up.Files' own order: every refused file rides along unchanged +// (ApplyUndo declines these itself, silently, exactly as it already does +// when handed the unfiltered plan - spec §10's refusal is not this task's +// to make noisier); every actionable file approved marks true rides along +// unchanged too. A file the user said no to, or left unmarked when [d] or +// [q] cut a per-file review short, is not dropped - fix round 2026-09-12, +// item 2 of Task 8's review: spec §9 says a declined file is logged even +// though nothing happens to it, the same as the forward path already does, +// so it is kept with Declined set, which tells ApplyUndo to log its steps +// as declined rather than reverse them. +func finalizeUndoPlan(up *engine.UndoPlan, approved map[int]bool) *engine.UndoPlan { + out := &engine.UndoPlan{Run: up.Run} + for i, f := range up.Files { + if f.Refused == "" && !approved[i] { + f.Declined = true + } + out.Files = append(out.Files, f) + } + return out +} + +// reviewUndoDir drives the interactive review over the real terminal, +// mirroring review.go's reviewDir for the forward path. +func reviewUndoDir(out io.Writer, files []engine.UndoFile) (map[int]bool, rune, error) { + return reviewUndoFiles(keyReader{stdin}, out, files) +} + +// reviewUndoFiles is spec §10's approval flow for an undo plan: the +// top-level +// +// [a] apply all [c] choose per file [s] skip [q] quit +// +// menu, and, for [c], the per-file +// +// [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing +// +// prompt - the same shape as review.go's reviewChains/reviewPerFile, over a +// different plan shape (approved is keyed by index into files, not by +// name). action is always one of 'a', 'c', 's' or 'q', with the same [q] +// folding rule reviewChains uses: a [c] session's own [q] becomes the same +// top-level 'q', and approved is emptied to match. +func reviewUndoFiles(in io.Reader, out io.Writer, files []engine.UndoFile) (map[int]bool, rune, error) { + fmt.Fprint(out, "\n[a] apply all [c] choose per file [s] skip [q] quit\n") + for { + key, err := readKey(in) + if err != nil { + return nil, 0, err + } + switch key { + case 'a': + return approveAllUndo(files), 'a', nil + case 's': + return map[int]bool{}, 's', nil + case 'q': + return map[int]bool{}, 'q', nil + case 'c': + approved, quit, err := reviewUndoPerFile(in, out, files) + if err != nil { + return nil, 0, err + } + if quit { + return map[int]bool{}, 'q', nil + } + return approved, 'c', nil + default: + fmt.Fprintf(out, "%q is not a, c, s or q\n", key) + } + } +} + +// reviewUndoPerFile is the per-file half of reviewUndoFiles. A file already +// refused at planning time is never asked about - spec §10 shows it with +// its reason and reverses nothing of it regardless of anything chosen here +// - but it still gets its own [i/N] line, so the numbering accounts for +// every file in the plan, not just the reversible ones. +func reviewUndoPerFile(in io.Reader, out io.Writer, files []engine.UndoFile) (approved map[int]bool, quit bool, err error) { + approved = map[int]bool{} + yesRest := false + for i, f := range files { + fmt.Fprintf(out, "\n[%d/%d] %s/%s\n", i+1, len(files), f.Dir, f.File) + for _, s := range f.Steps { + fmt.Fprintf(out, " %s\n", undoActionCell(s)) + } + if f.Refused != "" { + fmt.Fprintf(out, " refused: %s\n", f.Refused) + continue + } + if yesRest { + approved[i] = true + continue + } + + fmt.Fprint(out, " [y] yes [n] no [a] yes to this and all remaining [d] done, apply chosen so far [q] quit, apply nothing\n") + for { + key, kerr := readKey(in) + if kerr != nil { + return nil, false, kerr + } + switch key { + case 'y': + approved[i] = true + case 'n': + // leave unapproved + case 'a': + approved[i] = true + yesRest = true + case 'd': + return approved, false, nil + case 'q': + return nil, true, nil + default: + fmt.Fprintf(out, "%q is not y, n, a, d or q\n", key) + continue + } + break + } + } + return approved, false, nil +} + +// approveAllUndo approves every reversible file in files by index - [a] +// apply all, at either the top level or mid per-file review. A refused file +// is never marked true: nothing would happen to it anyway (ApplyUndo skips +// it unconditionally), and leaving it unmarked here keeps this function's +// contract simple - "true means ask ApplyUndo to reverse it" - rather than +// also being the thing that decides refused files ride along regardless +// (finalizeUndoPlan does that, independently of this map). +func approveAllUndo(files []engine.UndoFile) map[int]bool { + approved := make(map[int]bool, len(files)) + for i, f := range files { + if f.Refused == "" { + approved[i] = true + } + } + return approved +} + +// undoStepWidth is the widest undo action word (padCell aligns every +// arrow), matching render.go's actionKindWidth for the forward table. +const undoStepWidth = len("undo-displace") + +// undoActionCell renders one undo step: its own refusal reason when it has +// one (a sibling directory not yet empty for undo-mkdir - spec §10's one +// case where a step's own failure does not refuse its whole file), the +// directory removed for undo-mkdir (no destination to show), the file being +// trashed for undo-copy (fix wave item 3: its Dst is deliberately empty - +// trash.Put only chooses the entry name at execution time - so this is the +// one action with no path to point an arrow at; before this fix the cell +// rendered as a bare "undo-copy → ", the plan's one row that said +// nothing about what it would do to the user's file), or an arrow to where +// the step puts the file back, ~-abbreviated - undo has no single root the +// way a sort plan does (one run can span several directories), so there is +// no root-relative form to render here the way actionCell has. +func undoActionCell(s engine.UndoStep) string { + if s.Refused != "" { + return padCell(s.Action, undoStepWidth) + " refused: " + s.Refused + } + switch s.Action { + case "undo-mkdir": + return padCell(s.Action, undoStepWidth) + " " + xdg.Abbrev(s.Src) + case "undo-copy": + return padCell(s.Action, undoStepWidth) + " " + xdg.Abbrev(s.Src) + " → trash" + } + return padCell(s.Action, undoStepWidth) + " → " + xdg.Abbrev(s.Dst) +} + +// printUndoPlan renders an undo plan the way krino undo shows it, below the +// "krino: undo RUN" header line cmdUndo has already written: a counts line, +// then the numbered table, mirroring printPlan's shape for a sort plan +// (spec §10: "shown ... the same way"). +func printUndoPlan(w io.Writer, up *engine.UndoPlan) { + actionable := undoActionableCount(up.Files) + fmt.Fprintf(w, "%d files · %d to reverse · %d refused\n", len(up.Files), actionable, len(up.Files)-actionable) + if len(up.Files) == 0 { + return + } + fmt.Fprintln(w) + printUndoTable(w, up.Files) +} + +// undoRow is one line of the undo table: a file's first step (num and file +// set) or a continuation line (both blank), the same layout planRow uses +// for a sort plan. +type undoRow struct { + num, file, action string +} + +// undoRows turns files into table rows. A refused file gets exactly one +// row - there is nothing to reverse, so no per-step continuation lines - +// showing its reason in place of any step. +func undoRows(files []engine.UndoFile) []undoRow { + var rows []undoRow + for i, f := range files { + label := f.Dir + "/" + f.File + if f.Refused != "" { + rows = append(rows, undoRow{num: strconv.Itoa(i + 1), file: label, action: "refused: " + f.Refused}) + continue + } + for j, s := range f.Steps { + row := undoRow{action: undoActionCell(s)} + if j == 0 { + row.num = strconv.Itoa(i + 1) + row.file = label + } + rows = append(rows, row) + } + } + return rows +} + +// printUndoTable prints rows in the same #, file, action layout +// printPlanTable uses for a sort plan, reusing its column-width helpers +// (relWidth/padCell/padLeft/colWidth, render.go) rather than re-deriving +// them. +func printUndoTable(w io.Writer, files []engine.UndoFile) { + rows := undoRows(files) + nums := make([]string, len(rows)) + fls := make([]string, len(rows)) + for i, r := range rows { + nums[i], fls[i] = r.num, r.file + } + numW := colWidth(nums, 0) + fileW := relWidth(fls) + + fmt.Fprintf(w, " %s %s steps\n", padLeft("#", numW), padCell("file", fileW)) + for _, r := range rows { + fmt.Fprintf(w, " %s %s %s\n", padLeft(r.num, numW), padCell(r.file, fileW), r.action) + } +} + +// colourRefused highlights "refused:" in the terminal's own ANSI red (bold, +// slot 1 - never hex), the undo counterpart of sort.go's +// colourDeletePermanently: the one thing an undo plan singles out for +// attention is the file or step nothing will be reversed for. Same +// no-op-when-plain guarantee: with colour false this never touches the +// text, which is what keeps every escape byte out of a plan piped to a +// file or read by another tool. +func colourRefused(text string, colour bool) string { + if !colour { + return text + } + return strings.ReplaceAll(text, "refused:", "\x1b[1;31mrefused:\x1b[0m") +} |
