From 26c94eb3db62ec6eebbf8d22c11afe691d9520c4 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Sun, 13 Sep 2026 02:31:32 +0200 Subject: krino: release 0.0.1 — man pages, install, examples, cross and release, README, changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also: undo removes the directories its run created; a hardlink is never a duplicate of its own other name; a flag written before "undo" is honoured; --version prints no leading v. Duplicate conditions with different scopes not sharing an original is documented as a known limitation. --- internal/apply/apply.go | 2 +- internal/apply/apply_test.go | 42 +++++++ internal/apply/fs.go | 35 ++++++ internal/config/skel/krino.conf | 5 +- internal/config/skel/template.conf | 5 +- internal/dup/dup.go | 50 ++++++-- internal/dup/dup_test.go | 226 ++++++++++++++++++++++++++++++++++ internal/engine/apply.go | 118 ++++++++++++++++++ internal/engine/apply_test.go | 243 +++++++++++++++++++++++++++++++++++++ internal/engine/plan_bench_test.go | 108 +++++++++++++++++ internal/journal/read_test.go | 214 ++++++++++++++++++++++++++++++++ internal/tui/tui.go | 6 +- internal/tui/tui_test.go | 8 +- 13 files changed, 1040 insertions(+), 22 deletions(-) create mode 100644 internal/engine/plan_bench_test.go (limited to 'internal') diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 2cd52e4..f177a17 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -153,7 +153,7 @@ func runFileStep(step plan.Step) StepResult { case plan.Move: err = moveFile(step.Src, dst) case plan.Rename: - err = os.Rename(step.Src, dst) + err = renameFile(step.Src, dst) } if err != nil { return StepResult{Step: step, Status: "failed", Detail: err.Error(), Made: made, DisplacedEntry: displacedEntry} diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 64b0176..4bf6c30 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -250,3 +250,45 @@ func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { t.Errorf("Made = %v, want %v (outermost first)", got[0].Made, want) } } + +// TestMoveFileRefusesOccupiedDestination is item 16 (fix round 2026-09-12, +// plan 5 Task 2): moveFile must refuse an occupied destination on its own, +// not merely rely on runFileStep having already checked - the exact +// arrangement that produced plan 4's Task 5 Critical, where a helper that +// replaced silently was trusted because some caller had checked. Called +// directly, bypassing runFileStep's own pre-check entirely. +func TestMoveFileRefusesOccupiedDestination(t *testing.T) { + dir := t.TempDir() + src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) + dst := write(t, filepath.Join(dir, "y.pdf"), "already there", 0o644) + + if err := moveFile(src, dst); err == nil { + t.Fatal("moveFile overwrote an existing destination") + } + if b, err := os.ReadFile(src); err != nil || string(b) != "source" { + t.Errorf("moveFile touched its source: %q, %v", b, err) + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "already there" { + t.Errorf("moveFile touched its destination: %q, %v", b, err) + } +} + +// TestRenameFileRefusesOccupiedDestination is item 16's other half: +// runFileStep's bare os.Rename call for the Rename kind was just as +// unguarded in itself as moveFile was. renameFile is the helper that now +// carries the same independent guard, called directly here. +func TestRenameFileRefusesOccupiedDestination(t *testing.T) { + dir := t.TempDir() + src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) + dst := write(t, filepath.Join(dir, "y.pdf"), "already there", 0o644) + + if err := renameFile(src, dst); err == nil { + t.Fatal("renameFile overwrote an existing destination") + } + if b, err := os.ReadFile(src); err != nil || string(b) != "source" { + t.Errorf("renameFile touched its source: %q, %v", b, err) + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "already there" { + t.Errorf("renameFile touched its destination: %q, %v", b, err) + } +} diff --git a/internal/apply/fs.go b/internal/apply/fs.go index 205089f..823d5c8 100644 --- a/internal/apply/fs.go +++ b/internal/apply/fs.go @@ -108,6 +108,16 @@ func moveFile(src, dst string) error { if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { return err } + // Item 16 (fix round 2026-09-12, plan 5 Task 2): this guard must hold + // independently of runFileStep's own pre-check, layered rather than + // moved - the exact arrangement that produced plan 4's Task 5 Critical, + // where a helper that replaced silently was trusted because some caller + // had checked. Placed immediately before the operation that would + // otherwise clobber dst, the same way copyFile's own guard sits right + // before its rename into place. + if err := refuseIfExists(dst); err != nil { + return err + } err := os.Rename(src, dst) if err == nil { return nil @@ -121,6 +131,31 @@ func moveFile(src, dst string) error { return os.Remove(src) } +// renameFile renames src to dst, refusing on its own when dst already +// exists rather than trusting that a caller checked first (item 16, same +// reasoning as moveFile's guard above): a bare os.Rename silently replaces +// an occupied destination, and runFileStep's own pre-check must not be the +// only thing standing between a rename step and that. +func renameFile(src, dst string) error { + if err := refuseIfExists(dst); err != nil { + return err + } + return os.Rename(src, dst) +} + +// refuseIfExists reports an error naming dst if something is already there +// (os.Lstat succeeds, following no symlink), and propagates any other stat +// failure. A nil return means dst was confirmed absent at the moment of the +// check. +func refuseIfExists(dst string) error { + if _, err := os.Lstat(dst); err == nil { + return fmt.Errorf("destination already exists: %s", dst) + } else if !os.IsNotExist(err) { + return err + } + return nil +} + // maxSuffixAttempts bounds nextFreeName. internal/plan/conflict.go and // internal/trash/trash.go each have their own cap of the same size, for the // same reason given below: nextFreeName solves yet another, independent diff --git a/internal/config/skel/krino.conf b/internal/config/skel/krino.conf index 48780ea..dbd883b 100644 --- a/internal/config/skel/krino.conf +++ b/internal/config/skel/krino.conf @@ -1,8 +1,9 @@ ;; -*- mode: lisp -*- ;; vim: set ft=lisp : ;; -;; krino's main configuration. The syntax is explained in -;; docs/sexp-primer.md; every form is described in krino.conf(5). +;; krino's main configuration. The syntax is explained in sexp-primer.md, +;; installed as share/doc/krino/sexp-primer.md under the install prefix, +;; which is ~/.local by default; every form is described in krino.conf(5). ;; The directories to sort, in this order. Each NAME has its rules in ;; dirs/NAME.conf. Add one with: krino new NAME PATH diff --git a/internal/config/skel/template.conf b/internal/config/skel/template.conf index 867b681..c3af09d 100644 --- a/internal/config/skel/template.conf +++ b/internal/config/skel/template.conf @@ -1,8 +1,9 @@ ;; -*- mode: lisp -*- ;; vim: set ft=lisp : ;; -;; krino rules for one directory. The syntax is explained in -;; docs/sexp-primer.md; every form is described in krino.conf(5). +;; krino rules for one directory. The syntax is explained in sexp-primer.md, +;; installed as share/doc/krino/sexp-primer.md under the install prefix, +;; which is ~/.local by default; every form is described in krino.conf(5). (path "@PATH@") diff --git a/internal/dup/dup.go b/internal/dup/dup.go index e3c7424..32bcfb0 100644 --- a/internal/dup/dup.go +++ b/internal/dup/dup.go @@ -174,18 +174,47 @@ func (x *Index) Lookup(path string) (original string, dup bool, err error) { return path, false, nil } orig := x.candidates[x.original(identical)].path - return orig, orig != path, nil + if orig == path { + return orig, false, nil + } + // Spec §5.5: two names for one file are never duplicates of each other. + // The other name may be a hardlink, or path itself indexed a second time + // under a DIR that overlaps the scanned tree. identicalTo gives every + // member of the content class the same set, so every lookup elects the + // same original, and no name for that original's file is reported as a + // duplicate: its content always keeps at least one name. Portable: + // os.SameFile, never a Stat_t.Dev/Ino read (that field's type differs + // across freebsd/openbsd, which `make ci` vets). + origInfo, err := os.Lstat(orig) + if err != nil { + return "", false, err + } + pathInfo, err := os.Lstat(path) + if err != nil { + return "", false, err + } + if os.SameFile(origInfo, pathInfo) { + return orig, false, nil + } + return orig, true, nil } // identicalTo returns the indexes in group (which all share idx's size, // idx included) whose content matches candidates[idx]: same partial hash, -// then, only for those that collide, the same full hash. idx is the file -// Lookup was asked about; a failure hashing it propagates, since Lookup can -// answer nothing without it. A failure hashing any other candidate in group -// only removes that candidate from consideration: a vanished candidate -// (errors.Is fs.ErrNotExist) is dropped silently, any other failure is -// recorded on the Index (see recordCandidateError) so the caller can warn -// about it once matching is done. +// then, only for those that collide, the same full hash. Every candidate +// with identical bytes is included, whatever its path or inode: a hardlink +// of idx, and idx's own path indexed a second time under an overlapping +// extra directory, are both members. That keeps the set the same whichever +// member Lookup was asked about, so every member elects the same original; +// Lookup, not this function, decides that a name for the elected original's +// own file is not a duplicate of it. +// +// idx is the file Lookup was asked about; a failure hashing it propagates, +// since Lookup can answer nothing without it. A failure hashing any other +// candidate in group only removes that candidate from consideration: a +// vanished candidate (errors.Is fs.ErrNotExist) is dropped silently, any +// other failure is recorded on the Index (see recordCandidateError) so the +// caller can warn about it once matching is done. func (x *Index) identicalTo(idx int, group []int) ([]int, error) { idxPartial, err := x.partialHash(x.candidates[idx].path) if err != nil { @@ -219,9 +248,10 @@ func (x *Index) identicalTo(idx int, group []int) ([]int, error) { x.recordCandidateError(x.candidates[j].path, err) continue } - if jFull == idxFull { - same = append(same, j) + if jFull != idxFull { + continue } + same = append(same, j) } return same, nil } diff --git a/internal/dup/dup_test.go b/internal/dup/dup_test.go index 5d2621c..ed7c1a3 100644 --- a/internal/dup/dup_test.go +++ b/internal/dup/dup_test.go @@ -352,3 +352,229 @@ func TestExtraDirSymlinkNotFollowed(t *testing.T) { t.Errorf("symlinked extra dir was indexed despite the error: dup=%v orig=%s", dup, orig) } } + +// linked builds a scan.File for a hardlink of an already-put file: same +// inode, so same content and metadata by construction, under a new name. +func linked(t *testing.T, dir, name string, target scan.File) scan.File { + t.Helper() + p := filepath.Join(dir, name) + if err := os.Link(target.Path, p); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(p) + if err != nil { + t.Fatal(err) + } + return scan.File{Path: p, Rel: name, Name: name, Size: fi.Size(), ModTime: fi.ModTime()} +} + +// TestHardlinksAreNotDuplicatesOfEachOther is R10 (plan 5 Task 2, added to +// the task outside the brief): spec §5.5 groups duplicate candidates by size +// and hash, with no inode check, so two hardlinked names - one inode, byte- +// identical by construction - were judged a duplicate pair. A rule of +// (when (duplicate)) (delete) would then remove a name the user relies on +// even though nothing was ever actually copied. os.SameFile must stop a +// file being judged a duplicate of itself under another name. +func TestHardlinksAreNotDuplicatesOfEachOther(t *testing.T) { + d := t.TempDir() + a := put(t, d, "a.pdf", []byte("same content"), 0) + b := linked(t, d, "b.pdf", a) + + x, errs := NewIndex([]scan.File{a, b}, nil) + if len(errs) > 0 { + t.Fatal(errs) + } + if orig, dup := lookup(t, x, a); dup { + t.Errorf("a.pdf reported as a duplicate of its own hardlink b.pdf (orig=%s)", orig) + } + if orig, dup := lookup(t, x, b); dup { + t.Errorf("b.pdf reported as a duplicate of its own hardlink a.pdf (orig=%s)", orig) + } +} + +// TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile is R10's other +// direction, and the one a naive "same size+hash means never a duplicate" +// fix would get wrong: a.pdf and b.pdf are hardlinks of one inode, but +// c.pdf is a genuinely separate, byte-identical copy under an extra +// (duplicate "DIR") directory, so spec §5.5 prefers it as the original. +// Deleting a.pdf and b.pdf then leaves the content intact in c.pdf - they +// really are duplicates, of c.pdf, and must still be reported as such. +func TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile(t *testing.T) { + scanned, filed := t.TempDir(), t.TempDir() + a := put(t, scanned, "a.pdf", []byte("same content"), 0) + b := linked(t, scanned, "b.pdf", a) + put(t, filed, "c.pdf", []byte("same content"), 0) // extra-dir copy: preferred as the original regardless of mtime + + x, errs := NewIndex([]scan.File{a, b}, []string{filed}) + if len(errs) > 0 { + t.Fatal(errs) + } + cPath := filepath.Join(filed, "c.pdf") + if orig, dup := lookup(t, x, a); !dup || orig != cPath { + t.Errorf("a.pdf: dup=%v orig=%s, want a real duplicate of %s", dup, orig, cPath) + } + if orig, dup := lookup(t, x, b); !dup || orig != cPath { + t.Errorf("b.pdf: dup=%v orig=%s, want a real duplicate of %s", dup, orig, cPath) + } +} + +// TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate is R10 extended +// to extra-directory candidates: a candidate is never a duplicate of a +// candidate that is the same file, and extra-directory candidates are +// candidates - the rule is not scanned-vs-scanned only. This is the shape +// (duplicate "DIR") exists for: (duplicate "~/backup") means "the backup +// already holds a copy, so the local name can go", but if ~/backup/a.pdf is +// a hardlink of ~/dl/a.pdf, the backup holds no copy at all, just the same +// file under a second name - judging the scanned file a duplicate would +// delete the only copy while the user believes it is backed up. +// +// Lookup's os.SameFile check compares the elected original with the file +// asked about whether the original was scanned or found under an extra +// directory, so this case needs no special handling; the test pins it as +// its own named case. +func TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate(t *testing.T) { + scanned, backup := t.TempDir(), t.TempDir() + a := put(t, scanned, "a.pdf", []byte("same content"), 0) + backupPath := filepath.Join(backup, "a.pdf") + if err := os.Link(a.Path, backupPath); err != nil { + t.Fatal(err) + } + + x, errs := NewIndex([]scan.File{a}, []string{backup}) + if len(errs) > 0 { + t.Fatal(errs) + } + if orig, dup := lookup(t, x, a); dup { + t.Errorf("a.pdf reported as a duplicate of its own hardlink under the extra dir (orig=%s)", orig) + } +} + +// TestThreeWayHardlinksAreNotDuplicatesOfEachOther is the direct +// generalisation of TestHardlinksAreNotDuplicatesOfEachOther to N names for +// one inode: three names, one inode, no other copy anywhere - none of them +// is a duplicate of either of the others. No special-casing for N > 2: all +// three elect the same original, and Lookup's SameFile check finds each +// name to be that original's own file. +func TestThreeWayHardlinksAreNotDuplicatesOfEachOther(t *testing.T) { + d := t.TempDir() + a := put(t, d, "a.pdf", []byte("same content"), 0) + b := linked(t, d, "b.pdf", a) + c := linked(t, d, "c.pdf", a) + + x, errs := NewIndex([]scan.File{a, b, c}, nil) + if len(errs) > 0 { + t.Fatal(errs) + } + for _, f := range []scan.File{a, b, c} { + if orig, dup := lookup(t, x, f); dup { + t.Errorf("%s reported as a duplicate (orig=%s)", f.Name, orig) + } + } +} + +// dupVerdicts looks up every file in fs and returns the paths reported as +// not a duplicate, and the originals the duplicates were reported against. +func dupVerdicts(t *testing.T, x *Index, fs ...scan.File) (kept []string, origs map[string]string) { + t.Helper() + origs = make(map[string]string) + for _, f := range fs { + orig, dup := lookup(t, x, f) + if dup { + origs[f.Path] = orig + } else { + kept = append(kept, f.Path) + } + } + return kept, origs +} + +// TestExtraDirIsTheScannedRoot: (duplicate "DIR") where DIR is the scanned +// root itself, so every scanned file is also indexed as an extra-directory +// candidate under the same path. Two identical files are one duplicate and +// one original, never two duplicates of each other: a (delete) rule must +// leave one copy. +func TestExtraDirIsTheScannedRoot(t *testing.T) { + d := t.TempDir() + a := put(t, d, "a.pdf", []byte("same content"), 0) // older: the original + b := put(t, d, "b.pdf", []byte("same content"), 5) + + x, errs := NewIndex([]scan.File{a, b}, []string{d}) + if len(errs) > 0 { + t.Fatal(errs) + } + kept, origs := dupVerdicts(t, x, a, b) + if len(kept) != 1 || kept[0] != a.Path { + t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, a.Path, origs) + } + if origs[b.Path] != a.Path { + t.Errorf("b.pdf: original = %q, want %s", origs[b.Path], a.Path) + } +} + +// TestExtraDirInsideRecursiveRoot: DIR is a subdirectory of a recursively +// scanned root, the "the archive already holds a copy, so the loose one can +// go" shape. The copy under DIR is the original even though it is newer; +// only the copy outside DIR is a duplicate. +func TestExtraDirInsideRecursiveRoot(t *testing.T) { + d := t.TempDir() + archived := put(t, d, "Archive/x.pdf", []byte("same content"), 5) + loose := put(t, d, "x-copy.pdf", []byte("same content"), 0) // older, but not under DIR + + x, errs := NewIndex([]scan.File{archived, loose}, []string{filepath.Join(d, "Archive")}) + if len(errs) > 0 { + t.Fatal(errs) + } + kept, origs := dupVerdicts(t, x, archived, loose) + if len(kept) != 1 || kept[0] != archived.Path { + t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, archived.Path, origs) + } + if origs[loose.Path] != archived.Path { + t.Errorf("x-copy.pdf: original = %q, want %s", origs[loose.Path], archived.Path) + } +} + +// TestExtraDirIsAnAncestorOfTheRoot: DIR contains the scanned root, as +// (duplicate "~") would for a root under the home directory. +func TestExtraDirIsAnAncestorOfTheRoot(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "dl") + a := put(t, root, "a.pdf", []byte("same content"), 0) // older: the original + b := put(t, root, "b.pdf", []byte("same content"), 5) + + x, errs := NewIndex([]scan.File{a, b}, []string{parent}) + if len(errs) > 0 { + t.Fatal(errs) + } + kept, origs := dupVerdicts(t, x, a, b) + if len(kept) != 1 || kept[0] != a.Path { + t.Errorf("kept = %v, want exactly %s (duplicates: %v)", kept, a.Path, origs) + } + if origs[b.Path] != a.Path { + t.Errorf("b.pdf: original = %q, want %s", origs[b.Path], a.Path) + } +} + +// TestThreeCopiesWithOverlappingExtraDirKeepOne: two copies under DIR and a +// third outside it, all scanned. Exactly one of the three is not a +// duplicate, it lies under DIR, and every duplicate names that same file as +// its original. +func TestThreeCopiesWithOverlappingExtraDirKeepOne(t *testing.T) { + d := t.TempDir() + x1 := put(t, d, "Archive/x1.pdf", []byte("same content"), 3) + x2 := put(t, d, "Archive/x2.pdf", []byte("same content"), 4) + loose := put(t, d, "x-copy.pdf", []byte("same content"), 0) + + x, errs := NewIndex([]scan.File{x1, x2, loose}, []string{filepath.Join(d, "Archive")}) + if len(errs) > 0 { + t.Fatal(errs) + } + kept, origs := dupVerdicts(t, x, x1, x2, loose) + if len(kept) != 1 || kept[0] != x1.Path { + t.Fatalf("kept = %v, want exactly %s (duplicates: %v)", kept, x1.Path, origs) + } + for p, orig := range origs { + if orig != x1.Path { + t.Errorf("%s: original = %q, want %s", p, orig, x1.Path) + } + } +} diff --git a/internal/engine/apply.go b/internal/engine/apply.go index c74414d..878cb18 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "sort" "strings" "syscall" "time" @@ -660,6 +661,50 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // declined - a single pass, not two, so the two kinds of file interleave in // the log exactly as the run touched them, the same as Apply's own // approved-and-declined chains do. +// +// Task 1 (plan 5): after every file's reversal has been attempted, a second, +// run-wide pass retries the directory removals that were refused as +// non-empty. planUndoFile puts the undo-mkdir step for a shared destination +// on whichever file's chain first created it (spec §9: only the step that +// actually created a directory logs a "mkdir" entry, so only that file's +// reversal carries the matching undo-mkdir); when that file reverses first, +// its siblings are usually still inside, the removal is correctly refused as +// non-empty (spec §10), and - without this pass - nothing ever retries it, +// leaving empty directories behind even though every file came back. This +// mirrors planUndoFile's own undoProjection insight (see its comment) one +// level up: a removal judged too early is judging the wrong world, whether +// that "too early" is mid-file (what the projection fixes) or mid-run (what +// this retry fixes). +// +// The retry is a run-level tidy-up, never a re-run of a step: it does not +// touch what the first undo-mkdir attempt already logged (that entry, ok or +// failed, stands exactly as it was written), and a directory the retry does +// manage to remove gets an ADDITIONAL journal entry - never a rewrite - so +// the log never disagrees with reality (my ruling on the point the brief +// left open: spec §9 logs every step, and a directory removed while the log +// still says its removal was refused would be a false record). Because +// journal.ranAnyUndoStep already excludes "undo-mkdir" from what marks a run +// "(undone)", this extra "ok" entry cannot change that marking either - +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins +// it rather than assuming it. A retried removal is likewise never folded +// into ApplyResult: it is +// collected from candidates whose first attempt already went through +// tallyFile once (via isFileAffecting's exemption), and counting it again +// here would double-count a directory that failed once and then quietly +// tidied itself away. +// +// Candidates are collected only from directories this run's own reversal +// created - by construction, since every candidate comes from an undo-mkdir +// step, and an undo-mkdir step exists only for a directory the forward run's +// Made recorded - never a directory the retry merely happens to find empty. +// They are retried deepest path first (retryDirRemovals), so a nested +// directory - e.g. Work/Sub under Work - is removed before its +// now-possibly-empty parent, the same outermost-created/innermost-removed +// discipline logStep and undoFile already keep within one file's own chain, +// applied here across files. A directory still non-empty at retry time +// genuinely holds something else (or the retry runs before every sibling +// happens to have reversed, on a later undo of a different run) and simply +// stays, with its original refusal the only record of it. func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, run string) (*ApplyResult, error) { result := &ApplyResult{} @@ -695,6 +740,7 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, return result, fmt.Errorf("engine: apply undo: %w", err) } + var retries []dirRetry for _, f := range actionable { if err := ctx.Err(); err != nil { return result, err @@ -714,6 +760,15 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, } result.Files = append(result.Files, fr) tallyFile(result, fr.Steps, func(i int) bool { return isFileAffecting(f.Steps[i].Action) }) + for i, us := range f.Steps { + if us.Action == "undo-mkdir" && fr.Steps[i].Status == "failed" { + retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1}) + } + } + } + + if err := e.retryDirRemovals(j, run, retries); err != nil { + return result, fmt.Errorf("engine: apply undo: %w", err) } if err := j.Append(journal.Entry{Time: e.Now(), Run: run, Action: "run-end", Status: "ok"}); err != nil { @@ -722,6 +777,69 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, return result, nil } +// dirRetry names one directory whose undo-mkdir was refused (as non-empty) +// during ApplyUndo's main pass, kept for the run-wide retry once every +// file's reversal has been attempted. file and dirName are the file and +// config directory name that owned the original undo-mkdir step, carried +// forward so retryDirRemovals's journal entry - if the retry succeeds - +// names the same file and directory the original refusal did, not an +// arbitrary one; step is that same step's 1-based index, so the two entries +// (the original "failed" and, if the retry succeeds, this "ok") read +// together under the same File/Step in the log. +type dirRetry struct { + dir string + dirName string + file string + step int +} + +// retryDirRemovals is ApplyUndo's run-wide second pass (Task 1, plan 5): once +// every file's reversal has run, some directories an undo-mkdir step could +// not remove earlier may now be empty, because a sibling file that shared +// the directory has since reversed too. candidates is sorted deepest path +// first (by descending path-segment count) so a nested directory is removed +// before its parent, exactly the order a real cleanup needs; a directory +// still non-empty at its turn genuinely holds something else and is left +// exactly as its first attempt recorded it - no second entry, no error. +// +// This never rewrites or removes the original undo-mkdir entry (ok or +// failed, whichever the first attempt logged): a directory the retry does +// manage to remove gets one ADDITIONAL entry instead (my ruling on the point +// the brief left open - see ApplyUndo's comment), so the log always agrees +// with what is actually on disk. The new entry's own Action is still +// "undo-mkdir", so journal.ranAnyUndoStep - which excludes that action on +// principle, not by accident (see its own comment) - continues to treat this +// exactly like any other undo-mkdir for the purpose of marking a run +// "(undone)": tidying up an empty directory, on the first attempt or the +// retry, is still not a restoration. +func (e *Engine) retryDirRemovals(j *journal.Writer, run string, candidates []dirRetry) error { + sort.SliceStable(candidates, func(i, j int) bool { + return pathDepth(candidates[i].dir) > pathDepth(candidates[j].dir) + }) + for _, c := range candidates { + if err := os.Remove(c.dir); err != nil { + // Still not empty (or gone, or otherwise unremovable): the + // original refusal already recorded this, and it stands. + continue + } + if err := j.Append(journal.Entry{ + Time: e.Now(), Run: run, Dir: c.dirName, File: c.file, Step: c.step, + Action: "undo-mkdir", Status: "ok", Src: c.dir, + }); err != nil { + return err + } + } + return nil +} + +// pathDepth counts path's separators after cleaning it, so retryDirRemovals +// can sort deepest first: a nested directory (more separators) is always +// removed before the parent it sits under, whatever the two paths' common +// root. +func pathDepth(path string) int { + return strings.Count(filepath.Clean(path), string(filepath.Separator)) +} + // declineUndoFile logs f's reversal as declined without carrying out any of // it - spec §9's "declined files are logged even though nothing happens to // them", extended to undo (fix round 2026-09-12, item 2 of Task 8's review): diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index 42724a0..a54bffc 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -1180,3 +1180,246 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { t.Errorf("result = %+v, want one of each", result) } } + +// --- Plan 5, Task 1 --- + +// sharedDestUndoFixture builds a downloads directory with three pdf files +// (a.pdf, b.pdf, c.pdf) and a single rule moving all of them into dest, +// applies the move, and returns the sandbox home, the loaded engine, the +// journal's path and the forward run's ID. +// +// All three files landing on one destination that this one run creates is +// the shape that exercises the run-wide directory retry (Task 1, plan 5): +// whichever file's chain first creates dest carries its undo-mkdir step(s), +// and that file's own reversal typically runs while its siblings still +// occupy dest - refusing the removal correctly, at first. dest may name a +// nested path ("Work/Sub"): apply.mkdirAllTracked then records every +// directory the move had to create, outermost first, and every one of them +// still lands on that same first file's chain. +// +// Extracted per fix round 1 (Important 2): TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses +// and TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone +// used to duplicate this setup verbatim; TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst +// needed the identical shape with only dest varying, which is what named the +// parameter rather than hard-coding "Filed" here. +func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logPath string, run string) { + t.Helper() + h = sandbox(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + // Three files that all move into ONE created destination. The bug this + // task fixes is that only the file whose chain first creates it ever + // carries the undo-mkdir step, and that step is attempted while its + // siblings are still inside. + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if err := os.WriteFile(filepath.Join(dl, n), []byte(n), 0o640); err != nil { + t.Fatal(err) + } + } + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` +(path "~/dl") +(min-age 0s) +(rule "pdfs" (when (type pdf)) (move "` + dest + `")) +`}) + loaded, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + e = loaded + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + approved := map[string]bool{} + for _, c := range dp.Chains { + approved[c.File.Rel] = true + } + logPath = filepath.Join(h, ".local", "state", "krino", "krino.log") + j, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + run = journal.NewRunID(time.Now()) + if _, err := e.Apply(context.Background(), dp, approved, j, run); err != nil { + t.Fatal(err) + } + j.Close() + return h, e, logPath, run +} + +func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) { + h, e, logPath, run := sharedDestUndoFixture(t, "Filed") + dl := filepath.Join(h, "dl") + filed := filepath.Join(dl, "Filed") + if _, err := os.Stat(filed); err != nil { + t.Fatalf("apply did not create the directory: %v", err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + if _, err := os.Stat(filed); !os.IsNotExist(err) { + t.Errorf("undo left the created directory behind: %v", err) + } + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if _, err := os.Stat(filepath.Join(dl, n)); err != nil { + t.Errorf("%s did not come back: %v", n, err) + } + } +} + +// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's +// Important 1: retryDirRemovals must retry deepest path first. All three +// files move into Work/Sub, so Apply's single mkdirAllTracked call creates +// both Work and Work/Sub on the FIRST file's own chain (outermost first), +// which means that one file's reversal carries two undo-mkdir steps, one for +// each directory - and both are refused on that file's own turn, since the +// other two files still sit in Work/Sub at that point. +// +// Retried deepest first, Work/Sub empties out and is removed, and Work - +// now itself empty - is removed right after. Retried shallowest first +// instead, Work is tried while Work/Sub (now empty, but not yet removed) +// still sits inside it, so Work is refused as non-empty and never retried +// again in this run; Work/Sub is then removed, leaving the outer Work +// directory behind. So end state alone - no directory left over - already +// distinguishes correct (deepest-first) ordering from inverted or dropped +// ordering; unlike the flat-destination tests above, where only one +// directory ever entered `retries`, this is the case built to tell the two +// apart. +func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) { + h, e, logPath, run := sharedDestUndoFixture(t, "Work/Sub") + dl := filepath.Join(h, "dl") + work := filepath.Join(dl, "Work") + sub := filepath.Join(work, "Sub") + if _, err := os.Stat(sub); err != nil { + t.Fatalf("apply did not create the nested directory: %v", err) + } + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + res, err := e.ApplyUndo(context.Background(), up, j2, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + if _, err := os.Stat(sub); !os.IsNotExist(err) { + t.Errorf("undo left the nested directory behind: %v", err) + } + if _, err := os.Stat(work); !os.IsNotExist(err) { + t.Errorf("undo left the outer directory behind - retryDirRemovals is not retrying deepest path first: %v", err) + } + for _, n := range []string{"a.pdf", "b.pdf", "c.pdf"} { + if _, err := os.Stat(filepath.Join(dl, n)); err != nil { + t.Errorf("%s did not come back: %v", n, err) + } + } +} + +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone pins +// the journal half of the ruling on the point Task 1's brief left open: when +// retryDirRemovals succeeds in removing a directory, it appends an +// ADDITIONAL journal entry for it - the original "failed" undo-mkdir entry, +// recorded on whichever file's chain first created the directory, is never +// rewritten or removed - and, because that entry's Action is still +// "undo-mkdir" like the first, journal.ranAnyUndoStep continues to exclude +// it from what marks a run "(undone)" (the brief's constraint: it "cannot +// change whether a run shows as (undone); confirm that rather than assume +// it"). +func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) { + _, e, logPath, run := sharedDestUndoFixture(t, "Filed") + + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j2, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + undoRun := journal.NewRunID(time.Now()) + res, err := e.ApplyUndo(context.Background(), up, j2, undoRun) + if err != nil { + t.Fatal(err) + } + j2.Close() + if res.Failed != 0 { + t.Fatalf("undo reported %d failures: %+v", res.Failed, res) + } + + entries, err := journal.Entries(logPath, undoRun) + if err != nil { + t.Fatal(err) + } + var failed, ok *journal.Entry + var mkdirCount int + for i := range entries { + en := &entries[i] + if en.Action != "undo-mkdir" { + continue + } + mkdirCount++ + switch en.Status { + case "failed": + failed = en + case "ok": + ok = en + } + } + if mkdirCount != 2 { + t.Fatalf("undo-mkdir entries = %d, want exactly 2 (the original refusal plus the retry's addition): %+v", mkdirCount, entries) + } + if failed == nil { + t.Fatal("the original refused undo-mkdir entry is missing - it must never be rewritten or removed") + } + if ok == nil { + t.Fatal("no successful undo-mkdir entry was appended for the retry") + } + if failed.Src != ok.Src { + t.Errorf("failed.Src = %q, ok.Src = %q; want the same directory", failed.Src, ok.Src) + } + if failed.File != ok.File { + t.Errorf("failed.File = %q, ok.File = %q; want the retry entry to carry the file that owned the original undo-mkdir", failed.File, ok.File) + } + if failed.Dir != ok.Dir { + t.Errorf("failed.Dir = %q, ok.Dir = %q; want the same directory name", failed.Dir, ok.Dir) + } + + runs, err := e.Runs(0) + if err != nil { + t.Fatal(err) + } + byID := map[string]bool{} + for _, r := range runs { + byID[r.ID] = r.Undone + } + if !byID[run] { + t.Errorf("original run %q not marked Undone, though every file came back", run) + } + if byID[undoRun] { + t.Errorf("the undo run %q itself must never read as Undone", undoRun) + } +} diff --git a/internal/engine/plan_bench_test.go b/internal/engine/plan_bench_test.go new file mode 100644 index 0000000..a3d08bb --- /dev/null +++ b/internal/engine/plan_bench_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "krino/internal/plan" +) + +// BenchmarkPlan measures krino's own cost of planning: walking a tree, +// matching rules and building action chains (Engine.Plan, which wraps +// Engine.Match and plan.Build). It does NOT measure krino's real-world +// throughput - spec §13 is explicit that there is no performance target +// for 0.0.1, because a full run's wall time is dominated by the external +// extractors (pdftotext and friends), not by krino itself. This benchmark +// therefore uses a config with no (content ...) test, so no extractor +// ever runs and the result is identical on any machine, with or without +// poppler installed. +// +// The tree is built once in b.TempDir(), before the timer starts; each +// iteration re-plans the same on-disk tree with a fresh plan.Claims, so +// iterations are independent and repeatable. +func BenchmarkPlan(b *testing.B) { + root := b.TempDir() + scanRoot := filepath.Join(root, "Filed") + buildBenchTree(b, scanRoot) + + mainFile := filepath.Join(root, "krino.conf") + if err := os.WriteFile(mainFile, []byte(`(include "dl")`), 0o644); err != nil { + b.Fatal(err) + } + dirsDir := filepath.Join(root, "dirs") + if err := os.MkdirAll(dirsDir, 0o755); err != nil { + b.Fatal(err) + } + // Type-only rules (no content test), one per group present in the + // generated tree, mirroring examples/by-type.conf; "dat" files match + // none of them and take the unmatched path through Match. + dirConf := fmt.Sprintf(` +(path %q) +(recursive yes) +(min-age 0s) +(rule "images" (when (type image)) (move "Sorted/Images") (stop)) +(rule "documents" (when (type document)) (move "Sorted/Documents") (stop)) +(rule "spreadsheets" (when (type spreadsheet)) (move "Sorted/Spreadsheets") (stop)) +(rule "archives" (when (type archive)) (move "Sorted/Archives") (stop)) +(rule "media" (when (or (type audio) (type video))) (move "Sorted/Media") (stop)) +`, scanRoot) + if err := os.WriteFile(filepath.Join(dirsDir, "dl.conf"), []byte(dirConf), 0o644); err != nil { + b.Fatal(err) + } + + e, errs := Load(mainFile, "dl") + if len(errs) > 0 { + b.Fatalf("config errors: %v", errs) + } + + ctx := context.Background() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := e.Plan(ctx, e.Dirs[0], plan.NewClaims()); err != nil { + b.Fatal(err) + } + } +} + +// benchTreeDirs * benchFilesPerDir files are generated, spread over +// nested directories so the walk itself is exercised, not just a single +// flat directory read. +const ( + benchTreeDirs = 12 + benchFilesPerDir = 150 +) + +// buildBenchTree creates a synthetic tree under root for BenchmarkPlan: +// nested "Sub" directories holding files that cycle through extensions +// spanning several of Appendix A's type groups, plus one extension +// ("dat") that matches no rule. Names are neutral (Sub, a.) - +// never anything from a real folder, per the leak-check patterns. +func buildBenchTree(b *testing.B, root string) { + b.Helper() + exts := []string{"pdf", "jpg", "xlsx", "zip", "mp3", "dat"} + old := time.Now().Add(-time.Hour) + n := 0 + for d := 0; d < benchTreeDirs; d++ { + dir := filepath.Join(root, fmt.Sprintf("Sub%d", d), "Nested") + if err := os.MkdirAll(dir, 0o755); err != nil { + b.Fatal(err) + } + for f := 0; f < benchFilesPerDir; f++ { + ext := exts[n%len(exts)] + p := filepath.Join(dir, fmt.Sprintf("a%04d.%s", n, ext)) + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { + b.Fatal(err) + } + if err := os.Chtimes(p, old, old); err != nil { + b.Fatal(err) + } + n++ + } + } +} diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index 3ffa14d..2f0f40f 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -196,6 +196,220 @@ func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) { } } +// TestEntriesCrashedRunReturnsNilError is item 1: a run-start present, +// run-end absent, and otherwise clean is exactly the crashed-run shape +// Entries' own doc comment says it must accept - "to end of file when there +// is no run-end (a crashed run, which is precisely when corruption is +// likely)". Pinning it as its own test, rather than leaving it implicit in +// tests about something else, is the point of the item. +func TestEntriesCrashedRunReturnsNilError(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC) + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil { + t.Fatal(err) + } + // No run-end: the process crashed right here. + if err := w.Close(); err != nil { + t.Fatal(err) + } + + got, err := Entries(path, "A") + if err != nil { + t.Fatalf("a crashed but otherwise clean run returned an error: %v", err) + } + if len(got) != 2 || got[0].Action != "run-start" || got[1].Action != "move" { + t.Errorf("entries = %+v, want [run-start, move]", got) + } +} + +// TestEntriesIntactRunReturnsNilError is item 2: a complete, clean run - +// run-start, a step, run-end, nothing corrupt - must read back with a nil +// error. Every other test in this file needs this to be true along the way, +// but none of them state it as their own point; this one does. +func TestEntriesIntactRunReturnsNilError(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC) + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + + got, err := Entries(path, "A") + if err != nil { + t.Fatalf("a fully intact run returned an error: %v", err) + } + if len(got) != 3 || got[0].Action != "run-start" || got[1].Action != "move" || got[2].Action != "run-end" { + t.Errorf("entries = %+v, want [run-start, move, run-end]", got) + } +} + +// TestEntriesBothFailureModesReportsBadLineFirst is item 3: a run that both +// has an unparsable line inside its window AND lacks a readable run-start +// must surface as the unparsable-line error, not the missing-run-start one - +// Entries checks badLine before sawRunStart. The run-start line here is +// destroyed unattributably (as in TestEntriesFailsClosedOnMissingRunStart), +// and a second, still-attributable line is separately corrupted so badLine +// is set via the runFieldOf fallback rather than the window check. +func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC) + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + if len(lines) != 3 { + t.Fatalf("fixture has %d lines, want 3", len(lines)) + } + // Line 1 (run-start): destroyed unattributably - no tabs at all, so + // runFieldOf cannot even recover its Run column. + lines[0] = "totally-mangled-no-tabs-here" + // Line 2 (move): corrupt its Step column only - it keeps its tabs and + // its Run column ("A") stays readable via runFieldOf's fallback. + fields := strings.Split(lines[1], "\t") + fields[4] = "not-a-number" + lines[1] = strings.Join(fields, "\t") + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + got, err := Entries(path, "A") + if err == nil { + t.Fatal("Entries returned no error with both a bad line and a missing run-start present") + } + if !strings.Contains(err.Error(), "unparsable line 2") { + t.Errorf("error = %q, want it to report the unparsable line (line 2), not the missing run-start", err) + } + if len(got) != 1 || got[0].Action != "run-end" { + t.Errorf("entries = %+v, want just the surviving run-end", got) + } +} + +// TestEntriesAdjacentRunStartsOneCorrupted is item 4. Ruling R2: this pins +// what journal.Entries does TODAY for two runs whose run-start lines are +// adjacent, one of them corrupted - it does not assert an invented "correct" +// result, and internal/journal is not touched by this task. The log here is +// exactly: +// +// 1 run-start A (good) +// 2 run-start B (corrupted: no tabs, unattributable) +// 3 move A (good) +// 4 run-end A (good) +// 5 move B (good) +// 6 run-end B (good) +// +// Observed behaviour, traced by hand against Entries and confirmed by this +// test: Entries(path, "A") fails closed with the unparsable-line error, +// because line 2 falls inside A's own window (opened by line 1, not yet +// closed by a run-end) even though the corrupted line was actually B's +// run-start, not A's - this is exactly the "residual risk... a false +// refusal, not a false success" the function's own doc comment already +// names. Entries(path, "B"), in contrast, never sees line 2 as inside its +// window (B's window has not opened - its own run-start is the corrupted +// line), so it reaches the end of the file with no badLine, and instead +// fails on B's missing run-start. +// +// Concern (not fixed here, per R2 - flagged for judgement, not code +// change): the SAME corrupted line produces two different error shapes +// depending only on which run asks, which is a surprising inconsistency in +// the message a caller sees, even though both directions correctly fail +// closed. +func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 12, 8, 0, 0, 0, time.UTC) + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "B", Action: "run-start", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Dir: "dl", File: "x.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "A", Action: "run-end", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "B", Dir: "dl", File: "y.pdf", Step: 1, + Action: "move", Status: "ok", Src: "/a/y.pdf", Dst: "/b/y.pdf"}); err != nil { + t.Fatal(err) + } + if err := w.Append(Entry{Time: at, Run: "B", Action: "run-end", Status: "ok"}); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimRight(string(raw), "\n"), "\n") + if len(lines) != 6 { + t.Fatalf("fixture has %d lines, want 6", len(lines)) + } + // Line 2, B's run-start, adjacent to A's on line 1: destroyed + // unattributably. + lines[1] = "totally-mangled-no-tabs-here" + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + gotA, errA := Entries(path, "A") + if errA == nil { + t.Fatal("Entries(A) returned no error; pinned behaviour expects one (the corrupted adjacent line falls inside A's window)") + } + if !strings.Contains(errA.Error(), "unparsable line 2") { + t.Errorf("Entries(A) error = %q, want it to name the unparsable line 2", errA) + } + if len(gotA) != 3 || gotA[0].Action != "run-start" || gotA[1].Action != "move" || gotA[2].Action != "run-end" { + t.Errorf("Entries(A) = %+v, want A's own [run-start, move, run-end]", gotA) + } + + gotB, errB := Entries(path, "B") + if errB == nil { + t.Fatal("Entries(B) returned no error; pinned behaviour expects one (B's own run-start is the corrupted line)") + } + if !strings.Contains(errB.Error(), "no readable run-start") { + t.Errorf("Entries(B) error = %q, want it to name the missing run-start", errB) + } + if len(gotB) != 2 || gotB[0].Action != "move" || gotB[1].Action != "run-end" { + t.Errorf("Entries(B) = %+v, want B's surviving [move, run-end]", gotB) + } +} + // TestEntriesReportsAMangledLine: a corrupt line that is not the log's // final line must not be silently dropped by Entries the way Runs drops it // - PlanUndo needs to know a step went missing so it can refuse the whole diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 96bb728..c9f1241 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -36,9 +36,9 @@ func Colour(w io.Writer) bool { return !noColour } -// Height is the terminal's row count, 0 when it is not a terminal or the +// height is the terminal's row count, 0 when it is not a terminal or the // size cannot be read. -func Height(w io.Writer) int { +func height(w io.Writer) int { f, ok := w.(*os.File) if !ok || !isTerminal(int(f.Fd())) { return 0 @@ -73,7 +73,7 @@ func Page(w io.Writer, text string) error { // the terminal the pager would inherit, not the writer text is otherwise // sent to. func fitsWithoutPaging(text string) bool { - h := Height(os.Stdout) + h := height(os.Stdout) if h <= 0 { return true } diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 6918859..a3640ac 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -37,8 +37,8 @@ func TestColourNeedsTerminalAndNoNOCOLOR(t *testing.T) { func TestHeight(t *testing.T) { var buf bytes.Buffer - if h := Height(&buf); h != 0 { - t.Errorf("Height on a non-terminal writer = %d, want 0", h) + if h := height(&buf); h != 0 { + t.Errorf("height on a non-terminal writer = %d, want 0", h) } oldT, oldS := isTerminal, termSize @@ -46,8 +46,8 @@ func TestHeight(t *testing.T) { isTerminal = func(fd int) bool { return true } termSize = func(fd int) (int, int, error) { return 80, 24, nil } - if h := Height(os.Stdout); h != 24 { - t.Errorf("Height = %d, want 24", h) + if h := height(os.Stdout); h != 24 { + t.Errorf("height = %d, want 24", h) } } -- cgit v1.3