diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
| commit | bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd (patch) | |
| tree | 187e6a1fb722d9ab97d2076f69f997f5d371e943 /internal | |
| parent | cd7425b81f963a948f0abe7df3f9e58e190c2b78 (diff) | |
| download | krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.tar.gz krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.zip | |
comments that explain the code, not how it was written
About 340 comments cited the development process: task and plan
numbers, fix waves, rulings, reviewers, and the author in the third
person with a date. None of that exists outside the work itself, so to
a reader it pointed at nothing. Each one now states the engineering
reason it was standing in front of; where a comment was provenance and
nothing else, it is gone.
References to docs/design.md and docs/gui-design.md by section stay:
both ship with the repository. The design documents lose their
amendment diaries - CHANGELOG.md is that record - and the GUI's says
plainly that the window has gone further than the document.
Only comments changed. Every .go file was parsed and its code printed
with comments stripped, before and after: the two hashes are identical
across all 175 files.
Diffstat (limited to 'internal')
43 files changed, 644 insertions, 704 deletions
diff --git a/internal/apply/apply.go b/internal/apply/apply.go index a93aad4..d7bf71e 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -52,13 +52,13 @@ func Chain(c plan.Chain) []StepResult { // ChainLogged is Chain, calling done with each step's result as soon as // that step has run or been skipped, before the next one starts - so a // caller that logs from done never has a completed step missing from the -// log when the process dies mid-chain (review M9). An error from done stops -// the chain at once and is returned with the results so far. +// log when the process dies mid-chain. An error from done stops the chain +// at once and is returned with the results so far. // // A move or rename that had to take a free name at apply time, because its // planned destination was taken since planning, stops the chain too: every // later step was planned against the name the file did not get, and must -// not act on whatever is at that path (review M3). +// not act on whatever is at that path. // // Once ctx is cancelled (an interrupt), the step already under way finishes // and every later step is skipped as "interrupted": an interrupt stops after @@ -160,8 +160,8 @@ func runFileStep(step plan.Step) StepResult { var displacedEntry string if step.Displaces != "" { - // Re-checked at apply time (review M4): only a regular file may be - // trashed to make room, never a directory or link put there since. + // Re-checked at apply time: only a regular file may be trashed to + // make room, never a directory or link put there since. if fi, err := os.Lstat(step.Displaces); err != nil || !fi.Mode().IsRegular() { return StepResult{Step: step, Status: "failed", Detail: "the file to replace is gone or no longer a regular file"} } @@ -224,8 +224,8 @@ func runFileStep(step plan.Step) StepResult { // journal columns describe the file at Dst after the step, and after a // trash step the file genuinely lives there, so recording it is more // useful than an empty column and stays greppable. It also cannot confuse -// undo: Task 5's refusal condition for reversing a trash step is "the -// entry is gone, or Src now exists" — it reads Entry and Src, never Dst. +// undo: the refusal condition for reversing a trash step is "the entry is +// gone, or Src now exists" — it reads Entry and Src, never Dst. func runTrashStep(step plan.Step) StepResult { entry, err := trash.Put(step.Src) if err != nil { diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 4bf6c30..6134988 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -153,15 +153,15 @@ func TestChainSkippedStepIsNotAttempted(t *testing.T) { _ = time.Now } -// TestChainDisplacedFileRestoresFromDisplacedEntry is the fix-round-1 test: -// DisplacedEntry must be usable for undo, not merely present. It proves -// that by actually restoring the displaced file from the Trash and checking -// its content, not just that the field is non-empty. The displaced file -// sits at its own path, distinct from the step's own Dst: were the two the -// same (the ordinary overwrite shape), the mover's own file would already -// occupy that name by the time Restore ran, and Restore correctly refuses -// to land on an occupied path — this test isolates DisplacedEntry's own -// round-trip instead of also exercising that refusal. +// TestChainDisplacedFileRestoresFromDisplacedEntry: DisplacedEntry must be +// usable for undo, not merely present. It proves that by actually +// restoring the displaced file from the Trash and checking its content, +// not just that the field is non-empty. The displaced file sits at its own +// path, distinct from the step's own Dst: were the two the same (the +// ordinary overwrite shape), the mover's own file would already occupy +// that name by the time Restore ran, and Restore correctly refuses to land +// on an occupied path — this test isolates DisplacedEntry's own round-trip +// instead of also exercising that refusal. func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { root := t.TempDir() t.Setenv("HOME", root) @@ -205,12 +205,12 @@ func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { } } -// TestChainRunsRenameStep is the fix-round-2 gap: apply_test.go's only other -// Rename (in TestChainStopsWhenFileChanged) is always reported "skipped", -// because the Move before it is made to fail on purpose, so -// "case plan.Rename: err = os.Rename(step.Src, dst)" is never exercised by -// a passing test. A reversed-argument typo there would compile, pass every -// other test, pass make ci, and surface only as live data corruption. +// TestChainRunsRenameStep: apply_test.go's only other Rename (in +// TestChainStopsWhenFileChanged) is always reported "skipped", because the +// Move before it is made to fail on purpose, so "case plan.Rename: err = +// os.Rename(step.Src, dst)" is never exercised by a passing test. A +// reversed-argument typo there would compile, pass every other test, pass +// make ci, and surface only as live data corruption. func TestChainRunsRenameStep(t *testing.T) { root := t.TempDir() src := write(t, filepath.Join(root, "x.pdf"), "content", 0o644) @@ -229,12 +229,12 @@ func TestChainRunsRenameStep(t *testing.T) { } } -// TestChainMadeIsOutermostFirstForNestedDirectories is the fix-round-2 gap: -// every other test creates at most one missing directory level, so -// mkdirAllTracked's outermost-first ordering is correct by trace but -// unpinned by any assertion. Task 5 removes these directories in reverse, -// so a later accidental reordering would break undo while passing -// everything else here. +// TestChainMadeIsOutermostFirstForNestedDirectories: every other test +// creates at most one missing directory level, so mkdirAllTracked's +// outermost-first ordering is correct by trace but unpinned by any +// assertion. Undo removes these directories in reverse, so a later +// accidental reordering would break it while passing everything else +// here. func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { root := t.TempDir() write(t, filepath.Join(root, "x.pdf"), "content", 0o644) @@ -251,12 +251,11 @@ func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { } } -// 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. +// TestMoveFileRefusesOccupiedDestination: moveFile must refuse an occupied +// destination on its own, not merely rely on runFileStep having already +// checked - trusting that some caller had checked is exactly what let a +// silently replacing helper cause harm. 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) @@ -273,10 +272,10 @@ func TestMoveFileRefusesOccupiedDestination(t *testing.T) { } } -// 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. +// TestRenameFileRefusesOccupiedDestination: 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) diff --git a/internal/apply/fs.go b/internal/apply/fs.go index 56737e8..a8feccc 100644 --- a/internal/apply/fs.go +++ b/internal/apply/fs.go @@ -108,13 +108,12 @@ 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. + // This guard must hold independently of runFileStep's own pre-check, + // layered rather than moved: trusting that some caller already checked + // is exactly what lets a silently replacing helper cause harm. 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 } @@ -132,7 +131,7 @@ func moveFile(src, dst string) error { } // renameFile renames src to dst, refusing on its own when dst already -// exists rather than trusting that a caller checked first (item 16, same +// exists rather than trusting that a caller checked first (the 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. diff --git a/internal/cond/compile.go b/internal/cond/compile.go index 94118ac..6580ee9 100644 --- a/internal/cond/compile.go +++ b/internal/cond/compile.go @@ -32,8 +32,8 @@ func Compile(file string, when []*sexp.Node, opt Options) (*Cond, []*config.Diag return c, errs } -// compile is Compile with cost reordering switchable, for the property test -// in Task 8. +// compile is Compile with cost reordering switchable, for the property +// test. func compile(file string, when []*sexp.Node, opt Options, reorder bool) (*Cond, []*config.Diag) { c := &compiler{file: file, opt: opt, reorder: reorder, cond: &Cond{opt: opt}} var roots []*node diff --git a/internal/cond/eval.go b/internal/cond/eval.go index 1147c05..2c35dba 100644 --- a/internal/cond/eval.go +++ b/internal/cond/eval.go @@ -26,8 +26,7 @@ type Facts interface { Duplicate(dirs []string) (original string, ok bool, err error) // Matched reports whether an earlier rule matched this file, and whether // an earlier rule could not be decided (its condition was unknown): with - // no match and an undecided rule, (matched) is unknown (plan 11 review - // L6). + // no match and an undecided rule, (matched) is unknown. Matched() (matched, undecided bool) } @@ -40,12 +39,11 @@ type Result struct { // Unreadable is true when the condition's value is unknown: it depends // on a content test that could not read the file. Match is then false; - // an exclude holds anyway (review M11). A condition decided whatever the - // text holds - (and (content "x") (type txt)) on a pdf - is not - // unknown (plan 11). + // an exclude holds anyway. A condition decided whatever the text holds + // - (and (content "x") (type txt)) on a pdf - is not unknown. Unreadable bool // Undecided says what made the value unknown: "content unreadable", - // "duplicate check failed", or both, comma-separated (plan 12). + // "duplicate check failed", or both, comma-separated. Undecided string } @@ -95,8 +93,7 @@ func (ctx *evalCtx) undecidedLabel() string { } // undecidable reports whether a leaf whose fact could not be read is -// unknown rather than false: a content test (review M11) or a duplicate -// test (plan 12). +// unknown rather than false: a content test or a duplicate test. func undecidable(k kind) bool { return k == kContent || k == kDuplicate } diff --git a/internal/cond/eval_test.go b/internal/cond/eval_test.go index 7ebe038..73e0ea6 100644 --- a/internal/cond/eval_test.go +++ b/internal/cond/eval_test.go @@ -231,8 +231,8 @@ func TestNegatedCombinatorReason(t *testing.T) { // TestEvalReportsUnreadableContent: Result.Unreadable says a content test // was reached and could not read the file - what lets an exclude fail -// closed (review M11) - and stays false when evaluation never reached the -// content test. +// closed - and stays false when evaluation never reached the content +// test. func TestEvalReportsUnreadableContent(t *testing.T) { f := &fake{name: "a.pdf", rawErr: errors.New("larger than max-read")} if r := eval(t, `(and (type pdf) (content "x"))`, Options{}, f); r.Match || !r.Unreadable { @@ -264,8 +264,8 @@ func TestCapturesKeepDiacritics(t *testing.T) { // is unknown, not false, and and/or/not combine unknowns the way Kleene's // three-valued logic does: a condition certainly false (or true) whatever // the text holds is decided, and only one that depends on the text is -// unknown (plan 11, re-review cache F2). A rule matches only a true -// condition; an exclude holds on true or unknown. +// unknown. A rule matches only a true condition; an exclude holds on true +// or unknown. func TestUnreadableContentIsUnknown(t *testing.T) { f := &fake{name: "a.pdf", rawErr: errors.New("larger than max-read")} cases := []struct { @@ -289,8 +289,7 @@ func TestUnreadableContentIsUnknown(t *testing.T) { // TestMatchedIsUnknownAfterAnUnknownRule: when no earlier rule matched but // one could not be decided, (matched) is unknown, so a later -// (not (matched)) does not act on a file krino could not read (plan 11 -// review L6). +// (not (matched)) does not act on a file krino could not read. func TestMatchedIsUnknownAfterAnUnknownRule(t *testing.T) { f := &fake{name: "a.docx", matchedUnknown: true} if r := eval(t, `(not (matched))`, Options{}, f); r.Match || !r.Unreadable { @@ -303,7 +302,7 @@ func TestMatchedIsUnknownAfterAnUnknownRule(t *testing.T) { } // TestFailedDuplicateLookupIsUnknown: a duplicate test whose lookup fails is -// unknown, like unreadable content, and says why (plan 12). +// unknown, like unreadable content, and says why. func TestFailedDuplicateLookupIsUnknown(t *testing.T) { f := &fake{name: "a.pdf", dupErr: errors.New("~/dl/a.pdf: open: permission denied")} for _, c := range []struct { diff --git a/internal/cond/types.go b/internal/cond/types.go index 62a6640..97e761a 100644 --- a/internal/cond/types.go +++ b/internal/cond/types.go @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later // Package cond compiles the s-expression conditions of a rule's (when ...) -// into a tree that Task 8's evaluator walks against one file's facts. +// into a tree that the evaluator walks against one file's facts. package cond import ( @@ -21,7 +21,7 @@ type Options struct { // without when) is always true. type Cond struct { root *node // nil: always true - opt Options // the case/fold settings conditions were compiled with; Task 8 needs them again at eval time + opt Options // the case/fold settings conditions were compiled with; needed again at eval time UsesContent bool // some content test exists DupDirs [][]string // the raw directory arguments of each duplicate test, in order Keywords []Keyword // every content keyword, as compiled, in order @@ -91,7 +91,7 @@ type keyword struct { } // node is one compiled condition: a leaf test, or an and/or/not combinator -// over other nodes. Task 8 evaluates this tree. +// over other nodes. The evaluator walks this tree. type node struct { kind kind pos sexp.Pos // the position of the node as written, for diagnostics diff --git a/internal/config/enum_test.go b/internal/config/enum_test.go index 2e5b718..06f5640 100644 --- a/internal/config/enum_test.go +++ b/internal/config/enum_test.go @@ -13,7 +13,7 @@ import ( // with their String(), so every Conflict and CaseMode value must print as // the word a configuration uses, and parse back as that same value. A value // added later without its branch would otherwise print as "Conflict(3)", -// which krino check refuses (plan 13 review F5). +// which krino check refuses. func TestEverySettingValuePrintsAsItselfInAConfig(t *testing.T) { for _, c := range []struct { typ, form string diff --git a/internal/config/load.go b/internal/config/load.go index c75d000..3a1cbeb 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -24,9 +24,8 @@ type Config struct { // unusedOverrides reports text for a file no part of this configuration // names: it would otherwise pass as checked while the file on disk was read -// instead (plan 13 review F4). Text for an included directory this call did -// not load - an editor holding buffers for several while checking one - is -// not reported. +// instead. Text for an included directory this call did not load - an +// editor holding buffers for several while checking one - is not reported. func unusedOverrides(over map[string][]byte, used map[string]bool, mainFile string, include []string) []*Diag { known := map[string]bool{filepath.Clean(mainFile): true} for _, name := range include { diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 5f848af..a629ae5 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -72,9 +72,9 @@ func TestLoadErrors(t *testing.T) { } } -// TestLoadSyntaxErrorStopsAtOneDiag is item C: a krino.conf that fails to -// parse must report only the syntax error, not also "not in include" for -// names the caller asked for. +// TestLoadSyntaxErrorStopsAtOneDiag: a krino.conf that fails to parse must +// report only the syntax error, not also "not in include" for names the +// caller asked for. func TestLoadSyntaxErrorStopsAtOneDiag(t *testing.T) { root := t.TempDir() writeFiles(t, root, map[string]string{"krino.conf": `(include "dl"`}) @@ -146,7 +146,7 @@ func TestLoadWithOverriddenText(t *testing.T) { // TestLoadWithReportsAnUnusedOverride: an override whose path does not name // a file the load reads - a different spelling of it, or a directory not in // include - is reported, instead of the file on disk being read as though -// the unsaved text were fine (plan 13 review F4). +// the unsaved text were fine. func TestLoadWithReportsAnUnusedOverride(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) @@ -180,7 +180,7 @@ func diagText(ds []*Diag) string { // TestLoadWithOverrideForAnotherIncludedDirectory: checking one directory // while holding text for another included one is normal for an editor, and // not an error; only text for a file no configuration file names is -// reported (plan 13 review F4 follow-up). +// reported. func TestLoadWithOverrideForAnotherIncludedDirectory(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) @@ -201,8 +201,7 @@ func TestLoadWithOverrideForAnotherIncludedDirectory(t *testing.T) { } // TestLoadWithRefusesCollidingOverrides: two keys that name the same file -// would leave which text is read to map order, so they are refused (plan 13 -// review F4 follow-up). +// would leave which text is read to map order, so they are refused. func TestLoadWithRefusesCollidingOverrides(t *testing.T) { h := t.TempDir() t.Setenv("HOME", h) diff --git a/internal/config/print.go b/internal/config/print.go index a2244d0..6ae51e0 100644 --- a/internal/config/print.go +++ b/internal/config/print.go @@ -62,7 +62,7 @@ func PrintExclude(x *Exclude) string { // Pos and End - with text, and returns the new file contents. Every other // byte of src, comments and layout included, is kept exactly. Offsets that // do not belong to src - a form parsed from text that has since changed - -// are an error, not a panic (plan 13 review F6). +// are an error, not a panic. func Splice(src []byte, start, end sexp.Pos, text string) ([]byte, error) { if start.Offset < 0 || end.Offset < start.Offset || end.Offset > len(src) { return nil, fmt.Errorf("config: splice %d:%d is not inside %d bytes", start.Offset, end.Offset, len(src)) diff --git a/internal/config/print_test.go b/internal/config/print_test.go index 510fdd9..87b6878 100644 --- a/internal/config/print_test.go +++ b/internal/config/print_test.go @@ -87,8 +87,7 @@ func TestSpliceLeavesTheRestAlone(t *testing.T) { } // TestSpliceRefusesOffsetsOutsideTheSource: a stale end offset - the file -// changed since the form was parsed - is an error, not a panic (plan 13 -// review F6). +// changed since the form was parsed - is an error, not a panic. func TestSpliceRefusesOffsetsOutsideTheSource(t *testing.T) { src := []byte("(path \"/tmp\")") for _, c := range []struct{ start, end int }{{0, len(src) + 2}, {8, 4}, {-1, 3}} { diff --git a/internal/dup/dup_test.go b/internal/dup/dup_test.go index ed7c1a3..bd593de 100644 --- a/internal/dup/dup_test.go +++ b/internal/dup/dup_test.go @@ -368,13 +368,12 @@ func linked(t *testing.T, dir, name string, target scan.File) scan.File { 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. +// TestHardlinksAreNotDuplicatesOfEachOther: 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) @@ -392,13 +391,13 @@ func TestHardlinksAreNotDuplicatesOfEachOther(t *testing.T) { } } -// 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. +// TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile is the direction +// 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) @@ -418,15 +417,15 @@ func TestHardlinksAreStillDuplicatesOfASeparateIdenticalFile(t *testing.T) { } } -// 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. +// TestHardlinkUnderExtraDirWithNoOtherCopyIsNotADuplicate: 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 diff --git a/internal/engine/apply.go b/internal/engine/apply.go index 0d894c6..6c72a14 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -111,8 +111,8 @@ func (e *Engine) applyFile(ctx context.Context, dirName string, c plan.Chain, ap return FileResult{File: c.File, Steps: steps}, nil } - // Each step is logged the moment it has run (review M9), not after the - // whole chain: a run killed mid-chain must leave what it did undoable. + // Each step is logged the moment it has run, not after the whole chain: + // a run killed mid-chain must leave what it did undoable. results, err := apply.ChainLogged(ctx, c, func(i int, sr apply.StepResult) error { if err := e.logStep(j, run, dirName, rel, i+1, c.Steps[i], sr); err != nil { return unloggedStep(rel, c.Steps[i], sr, err) @@ -125,9 +125,9 @@ func (e *Engine) applyFile(ctx context.Context, dirName string, c plan.Chain, ap return FileResult{File: c.File, Steps: results}, nil } -// unloggedStep is the error for a step whose log entry could not be written -// (re-review N1). A step that ran is named with where its file is now: -// undo cannot see it, so the user must be told where to look. +// unloggedStep is the error for a step whose log entry could not be +// written. A step that ran is named with where its file is now: undo +// cannot see it, so the user must be told where to look. func unloggedStep(rel string, step plan.Step, sr apply.StepResult, err error) error { if sr.Status != "ok" { return fmt.Errorf("%s: step %s (%s) could not be logged: %w", rel, actionName(step.Kind), sr.Status, err) @@ -150,13 +150,12 @@ func unloggedStep(rel string, step plan.Step, sr apply.StepResult, err error) er // and Failed, matching each field's own "at least one step" definition. // // failureCounts, when non-nil, is asked before letting a "failed" status at -// index i count toward Failed. This is undo-mkdir's exemption (fix round 2, -// item 3): Task 7 maps ApplyResult to krino undo's exit code, and a file -// whose only failure is an undo-mkdir it correctly declined to remove (a -// shared directory not yet empty - not a hazard, see planUndoFile's and -// undoFile's comments) must not make the whole run look failed. The forward -// path passes nil: every one of its actions is file-affecting, so every -// failure counts. +// index i count toward Failed. This is undo-mkdir's exemption: ApplyResult +// maps to krino undo's exit code, and a file whose only failure is an +// undo-mkdir it correctly declined to remove (a shared directory not yet +// empty - not a hazard, see planUndoFile's and undoFile's comments) must +// not make the whole run look failed. The forward path passes nil: every +// one of its actions is file-affecting, so every failure counts. func tallyFile(result *ApplyResult, steps []apply.StepResult, failureCounts func(i int) bool) { var ok, failed, declined bool for i, sr := range steps { @@ -171,19 +170,18 @@ func tallyFile(result *ApplyResult, steps []apply.StepResult, failureCounts func declined = true } } - // Fix wave item 4 / Minor 6: a file every one of whose steps came back - // "skipped" - the shape an approved all-skipped chain used to take, one - // step for each rule action but every step's own Skip already set - - // left none of ok/failed/declined true above, so it fell out of the - // outcome tally entirely: "0 applied · 0 failed · 0 declined" for a - // file the user was asked about and approved. The converged - // actionableChains/countActing definition (cmd/krino, same fix wave - // item) keeps such a chain from ever reaching here approved in the - // first place, but tallyFile is the shared invariant, not a guarantee - // upheld only by that one caller: every file it is given must land in - // exactly one of the three buckets. Nothing ran and nothing failed, - // which is what "declined" already means to this tally, so an - // otherwise-uncounted file lands there. + // A file every one of whose steps came back "skipped" - the shape an + // approved all-skipped chain used to take, one step for each rule + // action but every step's own Skip already set - left none of + // ok/failed/declined true above, so it fell out of the outcome tally + // entirely: "0 applied · 0 failed · 0 declined" for a file the user was + // asked about and approved. The converged actionableChains/countActing + // definition (cmd/krino) keeps such a chain from ever reaching here + // approved in the first place, but tallyFile is the shared invariant, + // not a guarantee upheld only by that one caller: every file it is + // given must land in exactly one of the three buckets. Nothing ran and + // nothing failed, which is what "declined" already means to this + // tally, so an otherwise-uncounted file lands there. if !ok && !failed && !declined && len(steps) > 0 { declined = true } @@ -220,9 +218,9 @@ func actionName(k plan.Kind) string { // logStep writes every journal line one step produces: a "displace" entry // when the step trashed a file that was in its way (StepResult.DisplacedEntry -// is the only place that trash entry name exists — Task 3's ruling), a -// "mkdir" entry per directory the step actually created (outermost first, so -// undo can remove them innermost first), and finally the step's own entry. +// is the only place that trash entry name exists), a "mkdir" entry per +// directory the step actually created (outermost first, so undo can remove +// them innermost first), and finally the step's own entry. // All three share stepNum, the step's 1-based position in the chain, so a // reader can see which step of the plan a mkdir or displace line belongs to; // PlanUndo does not rely on that number, only on log order and File. @@ -241,11 +239,11 @@ func (e *Engine) logStep(j *journal.Writer, run, dirName, rel string, stepNum in if err := j.Append(journal.Entry{ Time: e.Now(), Run: run, Dir: dirName, File: rel, Step: stepNum, Action: "displace", Status: "ok", Rule: step.Rule, - // Detail carries the trash entry name explicitly (fix round 1, - // item 3): Dst's shape (trash.Dir()/files/<entry>) is - // internal/apply's and this file's own convention, not a - // contract undo may quietly depend on. PlanUndo/ApplyUndo read - // the name from here, never by taking Dst's basename. + // Detail carries the trash entry name explicitly: Dst's shape + // (trash.Dir()/files/<entry>) is internal/apply's and this + // file's own convention, not a contract undo may quietly + // depend on. PlanUndo/ApplyUndo read the name from here, + // never by taking Dst's basename. Src: step.Displaces, Dst: dst, Size: size, ModTime: mtime, Detail: sr.DisplacedEntry, }); err != nil { return err @@ -311,11 +309,10 @@ type UndoPlan struct { Files []UndoFile // Cleanup holds files with nothing left to reverse but directories the - // run made that something else occupied when the plan was built (re-review - // undo F3). They are not offered - that would repeat on every undo - but - // ApplyUndo removes any of those directories the other reversals leave - // empty, and logs it (plan 10 re-check R1). A front end that rebuilds the - // plan must carry Cleanup over. + // run made that something else occupied when the plan was built. They + // are not offered - that would repeat on every undo - but ApplyUndo + // removes any of those directories the other reversals leave empty, and + // logs it. A front end that rebuilds the plan must carry Cleanup over. Cleanup []UndoFile } @@ -331,14 +328,13 @@ type UndoFile struct { // Declined is never set by PlanUndo - it carries the front end's own // review decision back into ApplyUndo without widening ApplyUndo's - // signature (fix round 2026-09-12, item 2 of Task 8's review): true - // means the caller chose not to reverse an otherwise-reversible file - // (Refused empty), and ApplyUndo logs it exactly as a declined forward - // chain is logged (spec §9: "declined files are logged even though - // nothing happens to them") - one entry per step, status "declined" - - // rather than silently omitting it the way a Refused file still is. - // Setting this on a file that is also Refused has no effect: Refused's - // own silent-decline path is checked first and wins. + // signature: true means the caller chose not to reverse an otherwise- + // reversible file (Refused empty), and ApplyUndo logs it exactly as a + // declined forward chain is logged (spec §9: "declined files are logged + // even though nothing happens to them") - one entry per step, status + // "declined" - rather than silently omitting it the way a Refused file + // still is. Setting this on a file that is also Refused has no effect: + // Refused's own silent-decline path is checked first and wins. Declined bool } @@ -356,12 +352,12 @@ type UndoStep struct { // plan). // // journal.Entries returning a nil error is the only signal that runID's -// chain is intact (Task 1's ruling); a non-nil error, meaning a line inside -// the run's window failed to parse or the run has no readable run-start, -// refuses the whole run rather than build a reversal from a chain that might -// be missing steps. A run with no run-end (a crash) is not this case: -// Entries extends the window to end of file and still returns cleanly, so -// PlanUndo treats a crashed run exactly like an intact one. +// chain is intact; a non-nil error, meaning a line inside the run's window +// failed to parse or the run has no readable run-start, refuses the whole +// run rather than build a reversal from a chain that might be missing +// steps. A run with no run-end (a crash) is not this case: Entries extends +// the window to end of file and still returns cleanly, so PlanUndo treats +// a crashed run exactly like an intact one. func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { entries, err := journal.Entries(e.Config.LogFile(), runID) if err != nil { @@ -375,15 +371,15 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { } // Reversals an earlier undo of this same run already completed are not - // offered again (review M10): an undo that stopped part way can be - // finished by undoing the run once more. + // offered again: an undo that stopped part way can be finished by + // undoing the run once more. reversed, err := journal.ReversedSteps(e.Config.LogFile(), runID) if err != nil { return nil, fmt.Errorf("engine: plan undo: %w", err) } - // Entries are grouped by directory and file together (review M7): one run - // spans every directory, and two directories can each hold a file of the + // Entries are grouped by directory and file together: one run spans + // every directory, and two directories can each hold a file of the // same name. type fileKey struct{ dir, file string } var order []fileKey @@ -402,27 +398,27 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { up := &UndoPlan{Run: runID} for _, k := range order { uf := planUndoFile(k.dir, k.file, byFile[k], reversed) - // Critical finding, Task 8's review: a file every one of whose - // entries has Status != "ok" (declined by the ORIGINAL run's own - // review, or skipped, or failed before anything happened) yields - // an UndoFile with no Steps and no Refused - not a reversible - // file, and not a refused one either, just a file this run never - // touched. Appending it anyway lied about the plan: it counted - // toward "to reverse" (anything with Refused == "" does) while - // rendering no row and reversing nothing, so the final tally came - // up one short with no message. The condition is deliberately - // "no steps AND no refusal", never "no steps" alone - a - // permanently deleted file also has zero Steps, but planUndoFile - // sets Refused for it (spec §10: it must stay visible, with its - // reason, as not undoable), and that file must still be appended. + // A file every one of whose entries has Status != "ok" (declined + // by the ORIGINAL run's own review, or skipped, or failed before + // anything happened) yields an UndoFile with no Steps and no + // Refused - not a reversible file, and not a refused one either, + // just a file this run never touched. Appending it anyway lied + // about the plan: it counted toward "to reverse" (anything with + // Refused == "" does) while rendering no row and reversing + // nothing, so the final tally came up one short with no message. + // The condition is deliberately "no steps AND no refusal", never + // "no steps" alone - a permanently deleted file also has zero + // Steps, but planUndoFile sets Refused for it (spec §10: it must + // stay visible, with its reason, as not undoable), and that file + // must still be appended. if len(uf.Steps) == 0 && uf.Refused == "" { continue } if uf.Refused == "" && onlyOccupiedDirectoryRemovals(uf.Steps) { // Nothing of the file itself is left to reverse, only directories // the run made that something else still occupies: offering them - // would repeat on every undo (re-review undo F3). An empty one is - // still offered, and removed. + // would repeat on every undo. An empty one is still offered, and + // removed. up.Cleanup = append(up.Cleanup, uf) continue } @@ -455,8 +451,7 @@ func onlyOccupiedDirectoryRemovals(steps []UndoStep) bool { func isUndoRun(entries []journal.Entry) bool { any := false for _, en := range entries { - // A damaged line says nothing about which kind of run this is (plan - // 10 re-check R3). + // A damaged line says nothing about which kind of run this is. if en.Action == "run-start" || en.Action == "run-end" || en.Action == "damaged" { continue } @@ -487,30 +482,30 @@ func isFileAffecting(action string) bool { // innermost mkdir first; restore what it displaced last) — exactly the // order a real reversal needs. // -// Fix round 1, ruling on item 1: "undo-mkdir" is the one action excluded -// from the whole-file refusal gate, and the distinction is deliberate, not -// an inconsistency. Every other reversal's refusal condition - dst -// missing/changed, src now occupied, the trash entry gone - means the same -// thing: the world changed under us since the run, and reversing anyway -// could lose data. That is what spec §10's "no file is left half undone" -// exists to prevent, so it correctly gates the whole file. "Directory not -// empty" is not that kind of condition: it means a SIBLING file still lives -// there, which is not a hazard to anything, and a directory two files share -// is only actually empty once every file that used it has been reversed - -// checking it once at planning time, before any of those reversals have -// run, would refuse it (and, by the whole-file rule, the entire owning -// file, including its otherwise-safe undo-move) essentially every time two -// files share a destination directory, which is the common case. So an -// undo-mkdir reversal is never refused at planning time, and a failed one -// at execution time (ApplyUndo/undoFile) leaves the rest of that file's -// steps to run rather than aborting the file — the same "the substantive -// act's result is what is reported, cleanup is best-effort" shape as -// Task 2's .trashinfo ruling. isFileAffecting is the one predicate both -// this function and undoFile's stop-on-failure check share, so the two -// places this distinction matters cannot drift apart. +// "undo-mkdir" is the one action excluded from the whole-file refusal gate, +// and the distinction is deliberate, not an inconsistency. Every other +// reversal's refusal condition - dst missing/changed, src now occupied, the +// trash entry gone - means the same thing: the world changed under us since +// the run, and reversing anyway could lose data. That is what spec §10's +// "no file is left half undone" exists to prevent, so it correctly gates +// the whole file. "Directory not empty" is not that kind of condition: it +// means a SIBLING file still lives there, which is not a hazard to +// anything, and a directory two files share is only actually empty once +// every file that used it has been reversed - checking it once at planning +// time, before any of those reversals have run, would refuse it (and, by +// the whole-file rule, the entire owning file, including its otherwise-safe +// undo-move) essentially every time two files share a destination +// directory, which is the common case. So an undo-mkdir reversal is never +// refused at planning time, and a failed one at execution time +// (ApplyUndo/undoFile) leaves the rest of that file's steps to run rather +// than aborting the file — the same "the substantive act's result is what +// is reported, cleanup is best-effort" shape trash.Restore uses for its own +// .trashinfo cleanup (see its comment). isFileAffecting is the one +// predicate both this function and undoFile's stop-on-failure check share, +// so the two places this distinction matters cannot drift apart. // -// Fix wave item 1 (Critical): this loop owns an undoProjection, built up as -// it appends steps in the order they will actually execute. resolveConflict +// This loop owns an undoProjection, built up as it appends steps in the +// order they will actually execute. resolveConflict // (internal/plan/conflict.go) can make one contested path both a step's own // Dst and its Displaces - deliberately, and correct for the forward run - // which means the reversal that puts the incoming file back where it came @@ -557,9 +552,9 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R if k := (journal.ReversedKey{Dir: dir, File: file, Action: step.Action, Src: step.Src}); reversed[k] > 0 { // An earlier undo of this run already reversed this step: the // disk already shows it, and it is not offered again. It is not - // recorded in the projection either (re-review undo F1): what it - // put back is on disk now and is checked there, so a file changed - // since is refused rather than vouched for by the old reversal. + // recorded in the projection either: what it put back is on disk + // now and is checked there, so a file changed since is refused + // rather than vouched for by the old reversal. reversed[k]-- continue } @@ -569,7 +564,7 @@ func planUndoFile(dir, file string, ents []journal.Entry, reversed map[journal.R // against the disk itself. Judged against the disk as it is now, // en.Dst is empty - a later step of the chain moved the file on - // and every rename-then-move, move-then-move or move-then-trash - // chain would be refused (plan 8, found by the property test). + // chain would be refused (found by the property test). step.Refused = "" } if step.Refused == "" { @@ -650,13 +645,12 @@ func needsOccupancyCheck(action string) bool { // "changed since" and "trash entry is gone" checks); it never mutates // anything, so PlanUndo stays read-only. // -// Fix wave item 1: it deliberately does NOT decide the "src now exists" -// occupancy refusal any more - that is refuseIfSrcExists, called by -// planUndoFile's loop instead of from here. reverseStep is a pure function -// of one journal entry: it has no way to see the rest of the file's chain, -// so it cannot tell a real occupant from a path an earlier-executing step -// of this same chain is about to vacate. planUndoFile owns the projection -// that can. +// It deliberately does NOT decide the "src now exists" occupancy refusal +// any more - that is refuseIfSrcExists, called by planUndoFile's loop +// instead of from here. reverseStep is a pure function of one journal +// entry: it has no way to see the rest of the file's chain, so it cannot +// tell a real occupant from a path an earlier-executing step of this same +// chain is about to vacate. planUndoFile owns the projection that can. func reverseStep(en journal.Entry) UndoStep { us := UndoStep{Original: en, Action: "undo-" + en.Action} switch en.Action { @@ -685,16 +679,16 @@ func reverseStep(en journal.Entry) UndoStep { // // The mtime comparison is exact (time.Time.Equal), not truncated to whole // seconds: journal entries now round-trip through RFC3339Nano -// (journal.Writer.Append, fix round 1 item 4), which preserves the -// sub-second precision a fresh os.Stat also has. A whole-second comparison -// would let a file rewritten within the same second as the recorded mtime -// read as untouched, and undo would move it back believing it had not -// changed - the one guard that decides whether to overwrite the user's -// file, so it must not have that gap. -// Both refusal messages below go through xdg.Abbrev (Minor 4 / fix wave -// item 5): every step cell in the printed plan already abbreviates its path -// against $HOME (cmd/krino/undo.go's undoActionCell, via xdg.Abbrev), and a -// refusal reason sitting two lines under a "→ ~/dl/a.pdf" row in raw +// (journal.Writer.Append), which preserves the sub-second precision a +// fresh os.Stat also has. A whole-second comparison would let a file +// rewritten within the same second as the recorded mtime read as +// untouched, and undo would move it back believing it had not changed - +// the one guard that decides whether to overwrite the user's file, so it +// must not have that gap. +// Both refusal messages below go through xdg.Abbrev: every step cell in +// the printed plan already abbreviates its path against $HOME +// (cmd/krino/undo.go's undoActionCell, via xdg.Abbrev), and a refusal +// reason sitting two lines under a "→ ~/dl/a.pdf" row in raw // "/tmp/.../sbx/home/dl/a.pdf" form was the one cell that did not match. func refuseIfChanged(en journal.Entry) string { fi, err := os.Stat(en.Dst) @@ -707,10 +701,10 @@ func refuseIfChanged(en journal.Entry) string { return "" } -// refuseIfTrashChanged is the trash reversal's identity check (review M2): -// the entry must still be the file this run put there - the size and mtime -// the run logged for it - and its trashinfo must still record the path it -// came from. Emptying the Trash and trashing another file of the same name +// refuseIfTrashChanged is the trash reversal's identity check: the entry +// must still be the file this run put there - the size and mtime the run +// logged for it - and its trashinfo must still record the path it came +// from. Emptying the Trash and trashing another file of the same name // would otherwise have undo restore that file instead. func refuseIfTrashChanged(en journal.Entry) string { fi, err := os.Lstat(en.Dst) @@ -726,9 +720,9 @@ func refuseIfTrashChanged(en journal.Entry) string { return "" } -// recheck repeats, at execution time, the identity check PlanUndo made -// (review undo F8): an undo plan is shown and approved first, and a file -// changed in that window must not be moved back or trashed. +// recheck repeats, at execution time, the identity check PlanUndo made: an +// undo plan is shown and approved first, and a file changed in that window +// must not be moved back or trashed. func recheck(step UndoStep) string { switch step.Action { case "undo-move", "undo-rename", "undo-copy": @@ -741,11 +735,11 @@ func recheck(step UndoStep) string { // refuseIfSrcExists is the second half of every "reversal puts a file back // at a fixed path" refusal condition: reversing would silently clobber -// whatever is there now. Fix wave item 1: it is no longer reverseStep's own -// call (see reverseStep's comment) - planUndoFile calls it after reverseStep -// returns, passing the projection built from every reversal step already -// queued ahead of us in this same file's chain, so a path a predecessor is -// about to vacate does not read as occupied. needsOccupancyCheck excludes +// whatever is there now. It is no longer reverseStep's own call (see +// reverseStep's comment) - planUndoFile calls it after reverseStep returns, +// passing the projection built from every reversal step already queued +// ahead of us in this same file's chain, so a path a predecessor is about +// to vacate does not read as occupied. needsOccupancyCheck excludes // undo-copy (destination chosen by trash.Put at execution time) and // undo-mkdir (its own, execution-time-only refusal), the two actions whose // UndoStep.Dst is not a fixed path this check would even make sense against. @@ -762,9 +756,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // ApplyUndo reverses up, skipping every file whose Refused is set and // logging a declined file's steps without reversing them (see // declineUndoFile), and logs the reversal as a run of its own: a run-start -// whose Detail records which run this undoes (journal's own convention - -// Task 1 - so Runs can mark the original run Undone), one entry per undo -// step, and a run-end. +// whose Detail records which run this undoes (journal's own convention, so +// Runs can mark the original run Undone), one entry per undo step, and a +// run-end. // // Dir is left blank on the run-start/run-end entries: unlike Apply, which is // always scoped to one directory's DirPlan, one undo run can span several @@ -777,9 +771,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // 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 +// 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, @@ -795,9 +789,9 @@ func refuseIfSrcExists(us UndoStep, proj *undoProjection) string { // 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 +// the log never disagrees with reality (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 @@ -829,16 +823,14 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, result.Declined++ continue } - // Fix wave item 4 / final-wave item 24: the same invariant - // PlanUndo's own "no steps AND no refusal" guard states explicitly - // (see its comment) - a file with no steps and no Refused is one - // this run never touched, not a reversible one - given the same - // two-condition form here, rather than relying on the Refused - // branch above to have already made f.Refused == "" true by the - // time this runs. Written this way, the guard is correct on its - // own, independent of that branch's order or presence, rather than - // unreachable-by-construction the way the parked ruling on this - // line described it before Minor 6 showed the same error live. + // The same invariant PlanUndo's own "no steps AND no refusal" guard + // states explicitly (see its comment) - a file with no steps and no + // Refused is one this run never touched, not a reversible one - + // given the same two-condition form here, rather than relying on + // the Refused branch above to have already made f.Refused == "" + // true by the time this runs. Written this way, the guard is + // correct on its own, independent of that branch's order or + // presence. if len(f.Steps) == 0 && f.Refused == "" { continue } @@ -920,8 +912,8 @@ type dirRetry struct { log bool } -// 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 +// retryDirRemovals is ApplyUndo's run-wide second pass: 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 @@ -931,9 +923,9 @@ type dirRetry struct { // // 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 +// manage to remove gets one ADDITIONAL entry instead (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 @@ -972,13 +964,13 @@ func pathDepth(path string) int { // 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): -// a file the front end's own review chose not to reverse still gets one -// entry per step, status "declined", the same shape applyFile already gives -// a declined forward chain. Every step is declined, not just the first: an -// undo file whose reversal was never started needs the same per-step record -// a partially-run one would have, so a reader scanning the log by step -// number sees a complete, if inert, chain rather than a gap. +// them", extended to undo: a file the front end's own review chose not to +// reverse still gets one entry per step, status "declined", the same shape +// applyFile already gives a declined forward chain. Every step is +// declined, not just the first: an undo file whose reversal was never +// started needs the same per-step record a partially-run one would have, +// so a reader scanning the log by step number sees a complete, if inert, +// chain rather than a gap. func (e *Engine) declineUndoFile(f UndoFile, j *journal.Writer, run string) (FileResult, error) { steps := make([]apply.StepResult, len(f.Steps)) for i, step := range f.Steps { @@ -997,8 +989,8 @@ func (e *Engine) declineUndoFile(f UndoFile, j *journal.Writer, run string) (Fil // undoFile executes every step of f in order (already last-original-step // first from PlanUndo) and logs each. // -// Fix round 1, ruling on item 2: a failed FILE-AFFECTING step (everything -// but undo-mkdir - see isFileAffecting) stops the rest of the file's steps, +// A failed FILE-AFFECTING step (everything but undo-mkdir - see +// isFileAffecting) stops the rest of the file's steps, // matching apply.Chain's forward model, exactly because continuing past it // is the half-undone state spec §10 forbids: if undo-move fails, reversing // this file's still-earlier steps anyway would leave it in a state that was @@ -1059,8 +1051,7 @@ func runUndoStep(step UndoStep) apply.StepResult { case "undo-move", "undo-rename": // The directories created here are returned in Made: ApplyUndo removes // them again once empty, since an earlier file's undo-mkdir may already - // have removed the directory this file passes back through (review - // undo F6). + // have removed the directory this file passes back through. made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst)) if err != nil { return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} @@ -1082,12 +1073,12 @@ func runUndoStep(step UndoStep) apply.StepResult { case "undo-trash", "undo-displace": // The trash entry name is read from Original.Detail, where logStep - // put it explicitly (fix round 1, item 3) - never re-derived from - // Src or Dst's shape, which belong to internal/apply's and this - // file's own conventions and must stay free to change independently. - // The directory the file goes back into is created here, tracked, - // rather than silently by trash.Restore, so ApplyUndo removes it again - // when it ends up empty (review undo F6). + // put it explicitly - never re-derived from Src or Dst's shape, + // which belong to internal/apply's and this file's own conventions + // and must stay free to change independently. The directory the + // file goes back into is created here, tracked, rather than + // silently by trash.Restore, so ApplyUndo removes it again when it + // ends up empty. made, err := apply.MkdirAllTracked(filepath.Dir(step.Dst)) if err != nil { return apply.StepResult{Status: "failed", Detail: err.Error(), Made: made} @@ -1114,8 +1105,8 @@ func runUndoStep(step UndoStep) apply.StepResult { // Chain, so undo cannot reach its careful temp-file machinery and carries a // small, independent implementation instead. // -// The Lstat guard below is not optional (fix round 2, item 1, Critical): -// POSIX rename(2) replaces an existing regular file at dst without error, +// The Lstat guard below is not optional: POSIX rename(2) replaces an +// existing regular file at dst without error, // and PlanUndo's own "src now exists" check ran at planning time, not now - // spec §10 has an undo plan "shown and approved the same way" as any other, // a real human-length window in which something can create a file at dst @@ -1175,9 +1166,8 @@ func copyThenRemove(src, dst string) error { tmp.Close() return err } - // Fix round 2, item 2 (Important): sync before close, matching - // internal/apply's copyFile (fs.go), which this was modelled on - same - // durability requirement, same reason. + // Sync before close, matching internal/apply's copyFile (fs.go), which + // this was modelled on - same durability requirement, same reason. if err := tmp.Sync(); err != nil { tmp.Close() return err diff --git a/internal/engine/apply_test.go b/internal/engine/apply_test.go index 142e4d2..6c724b1 100644 --- a/internal/engine/apply_test.go +++ b/internal/engine/apply_test.go @@ -20,14 +20,12 @@ import ( // applyFixture builds a directory with two files and a rule moving pdfs into // Work, then plans it. It returns the home, the plan and an open journal. // -// Adapted from the brief to this package's actual writeConfig helper, which -// takes a main-file body and a dirs map keyed by name (see -// TestLoadRejectsUnsuppliedCaptures's comment in engine_test.go for the same -// adaptation elsewhere in this package): the brief's fixture wrote -// `(path ...)` and `(rule ...)` straight into what it called the main file, -// but the real config language (docs/design.md §4.2-4.3) requires those in a -// directory file reached through `(include ...)`. Every assertion below is -// unchanged from the brief; only this setup plumbing differs. +// This uses writeConfig, which takes a main-file body and a dirs map keyed +// by name (see TestLoadRejectsUnsuppliedCaptures's comment in +// engine_test.go for the same shape used elsewhere in this package): the +// real config language (docs/design.md §4.2-4.3) requires `(path ...)` and +// `(rule ...)` in a directory file reached through `(include ...)`, not in +// the main file directly. func applyFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -205,25 +203,24 @@ func TestPlanUndoRefusesPermanentDelete(t *testing.T) { } } -// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete is the Critical -// finding from Task 8's review: a file the ORIGINAL forward run declined -// (spec §9: its steps are still logged, status "declined") has no "ok" -// entries at all, so planUndoFile's last-to-first walk skips every one of -// them and returns an UndoFile with Steps == nil and Refused == "" - a file -// that was never touched, not a reversible one. Before the fix, PlanUndo -// appended that empty UndoFile anyway, and undoActionableCount (cmd/krino) -// counts every Refused == "" file as "to reverse" regardless of whether it -// has any steps - inflating the header's count while the table renders no -// row for it and the final tally comes up one short, silently, at exit 0. +// TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete: a file the +// ORIGINAL forward run declined (spec §9: its steps are still logged, +// status "declined") has no "ok" entries at all, so planUndoFile's +// last-to-first walk skips every one of them and returns an UndoFile with +// Steps == nil and Refused == "" - a file that was never touched, not a +// reversible one. Without the guard against that, PlanUndo would append +// that empty UndoFile anyway, and undoActionableCount (cmd/krino) counts +// every Refused == "" file as "to reverse" regardless of whether it has +// any steps - inflating the header's count while the table renders no row +// for it and the final tally comes up one short, silently, at exit 0. // -// The two halves in one test, deliberately, per the review: a fix that -// dropped every zero-step UndoFile instead of the correct +// The two halves live in one test, deliberately: a fix that dropped every +// zero-step UndoFile instead of the correct // "len(Steps) == 0 && Refused == \"\"" condition would also drop a // permanently deleted file (zero steps, but Refused IS set - spec §10 // requires it to stay visible with its reason) - so both conditions live // in the same test, and a future "simplification" that breaks either one -// fails this one test immediately rather than needing two separate reviews -// to notice. +// fails this one test immediately. func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -294,10 +291,9 @@ func TestPlanUndoDropsDeclinedFileButKeepsPermanentDelete(t *testing.T) { } } -// TestPlanUndoAcceptsIntactRun pins the trust Task 1 established but never -// itself exercised through PlanUndo: journal.Entries returning a nil error -// for a run whose run-start and run-end both parsed cleanly is the signal -// that the chain is intact, and PlanUndo must build a usable plan from it +// TestPlanUndoAcceptsIntactRun: journal.Entries returning a nil error for a +// run whose run-start and run-end both parsed cleanly is the signal that +// the chain is intact, and PlanUndo must build a usable plan from it // rather than refuse. func TestPlanUndoAcceptsIntactRun(t *testing.T) { h, e, dp, j, run := applyFixture(t) @@ -326,8 +322,8 @@ func TestPlanUndoAcceptsIntactRun(t *testing.T) { // TestPlanUndoAcceptsCrashedRun: a run-start with no run-end (the process // died mid-run) must still yield a usable undo plan, per Entries' documented -// window-to-EOF behaviour. If this refused, Task 1's contract and this -// task's assumption would disagree - worth a ruling, not a workaround. +// window-to-EOF behaviour. A refusal here would mean PlanUndo's assumption +// and Entries' actual behaviour have drifted apart. func TestPlanUndoAcceptsCrashedRun(t *testing.T) { h, e, dp, j, run := applyFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { @@ -424,7 +420,7 @@ func TestApplyChecksContextBetweenFilesNotWithinOne(t *testing.T) { } // TestApplyUndoRestoresMovedFile: the smallest possible round trip through -// ApplyUndo, since Task 9's is the only other test that exercises it. +// ApplyUndo. func TestApplyUndoRestoresMovedFile(t *testing.T) { h, e, dp, j, run := applyFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"a.pdf": true}, j, run); err != nil { @@ -543,8 +539,8 @@ func TestApplyUndoSkipsRefusedFiles(t *testing.T) { } } -// TestApplyUndoLogsDeclinedFile is fix round 2026-09-12, item 2 of Task 8's -// review: a file the front end's own review chose not to reverse (Refused +// TestApplyUndoLogsDeclinedFile: a file the front end's own review chose +// not to reverse (Refused // empty, Declined set by the caller - PlanUndo itself never sets it) must // still be logged, spec §9's "declined files are logged even though nothing // happens to them" extended to undo. The file must come back untouched, the @@ -620,9 +616,8 @@ func TestApplyUndoLogsDeclinedFile(t *testing.T) { } } -// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone is fix wave -// item 2 (Important): reproduced by the reviewer via pty as `1 moved -// (undone)` with the file still filed. The mechanism is +// TestApplyUndoDecliningEveryFileDoesNotMarkOriginalRunUndone: reproduced +// via pty as `1 moved (undone)` with the file still filed. The mechanism is // journal.Runs' own (see TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined // for that unit-level pin); this is the same defect exercised end to end // through a real forward run, a real declined undo, and e.Runs() itself - @@ -693,12 +688,10 @@ func TestRunsDelegatesToJournal(t *testing.T) { } } -// --- Fix round 1 --- - -// TestApplyLogsTrashEntryNameInDetail: fix round 1, item 3. The trash entry -// name must be logged explicitly (Detail), not left to be re-derived from -// Dst's basename - Dst's shape is internal/apply's contract, not undo's, and -// the two must not be secretly coupled. +// TestApplyLogsTrashEntryNameInDetail: the trash entry name must be logged +// explicitly (Detail), not left to be re-derived from Dst's basename - +// Dst's shape is internal/apply's contract, not undo's, and the two must +// not be secretly coupled. func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -773,10 +766,10 @@ func TestApplyLogsTrashEntryNameInDetail(t *testing.T) { } } -// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: fix round 1, item 3, -// isolated. Src is deliberately a path whose basename names no real trash -// entry; only Original.Detail names the real one. If runUndoStep ever goes -// back to deriving the name from Dst (or Src), this fails. +// TestRunUndoStepTrashReadsEntryNameFromDetailNotDst: Src is deliberately a +// path whose basename names no real trash entry; only Original.Detail +// names the real one. If runUndoStep ever goes back to deriving the name +// from Dst (or Src), this fails. func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { h := sandbox(t) dl := filepath.Join(h, "dl") @@ -794,7 +787,7 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { // Original is what the run logged for the trash step: the entry name in // Detail, and the entry's own path, size and mtime (checked again at - // execution time, review undo F8). + // execution time). entryPath := filepath.Join(trash.Dir(), "files", entry) fi, err := os.Stat(entryPath) if err != nil { @@ -815,8 +808,8 @@ func TestRunUndoStepTrashReadsEntryNameFromDetailNotDst(t *testing.T) { } } -// TestPlanUndoRefusesFileModifiedWithinSameSecond: fix round 1, item 4. The -// journal now records ModTime with sub-second precision (RFC3339Nano), so a +// TestPlanUndoRefusesFileModifiedWithinSameSecond: the journal now records +// ModTime with sub-second precision (RFC3339Nano), so a // file rewritten within the same whole second as the run must still be // detected as changed - a .Unix()-granularity comparison would miss this // and undo would silently move the edited file back over the user's data. @@ -851,8 +844,8 @@ func TestPlanUndoRefusesFileModifiedWithinSameSecond(t *testing.T) { } } -// TestUndoFileStopsAfterFailedFileAffectingStep: fix round 1, item 2. A -// failed undo-move must stop the rest of that file's reversal - continuing +// TestUndoFileStopsAfterFailedFileAffectingStep: a failed undo-move must +// stop the rest of that file's reversal - continuing // would leave it half undone (spec §10), even though the later step // (undo-copy) would, in isolation, have succeeded. func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { @@ -891,9 +884,9 @@ func TestUndoFileStopsAfterFailedFileAffectingStep(t *testing.T) { } } -// TestUndoFileContinuesPastFailedMkdir: fix round 1, item 2's other half - -// a failed undo-mkdir (directory not empty) must NOT stop the rest of the -// file's reversal, unlike every other action. +// TestUndoFileContinuesPastFailedMkdir: a failed undo-mkdir (directory not +// empty) must NOT stop the rest of the file's reversal, unlike every other +// action. func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { h := sandbox(t) nonEmpty := filepath.Join(h, "nonempty") @@ -940,11 +933,9 @@ func TestUndoFileContinuesPastFailedMkdir(t *testing.T) { } } -// --- Fix round 2 --- - -// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: fix round 2, -// item 1 (Critical). Spec §10 says an undo plan is shown and approved like -// any other, so there is a real, human-length window between PlanUndo's +// TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution: spec §10 +// says an undo plan is shown and approved like any other, so there is a +// real, human-length window between PlanUndo's // refuseIfSrcExists check and ApplyUndo actually running - long enough for // something else to create a file at the reversal's destination in between. // undo-move/undo-rename must re-check at execution time rather than let a @@ -993,11 +984,10 @@ func TestApplyUndoRefusesWhenDestinationReappearsBeforeExecution(t *testing.T) { } } -// TestApplyUndoDoesNotCountFailedMkdirAsFailed: fix round 2, item 3. A file -// whose only failure is an undo-mkdir (a shared directory not yet empty) -// must not flip ApplyResult.Failed - Task 7 maps that to krino undo's exit -// code, and ruling 4 (fix round 1, item 1) established that this specific -// refusal is tidiness, not a hazard. +// TestApplyUndoDoesNotCountFailedMkdirAsFailed: a file whose only failure +// is an undo-mkdir (a shared directory not yet empty) must not flip +// ApplyResult.Failed - ApplyResult maps to krino undo's exit code, and +// this specific refusal is tidiness, not a hazard. func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { h := sandbox(t) dir := filepath.Join(h, "Work") @@ -1044,15 +1034,13 @@ func TestApplyUndoDoesNotCountFailedMkdirAsFailed(t *testing.T) { } } -// --- Fix wave (2026-09-12) --- - // overwriteFixture builds a directory where a forward move under // (on-conflict overwrite) will displace a pre-existing file at its // destination: dl/incoming.pdf moves to dl/Work/incoming.pdf, which already // holds a different file (the "victim") the move must trash first. This is // the one shape that makes a step's Displaces and another step's Dst name // the exact same path (internal/plan/conflict.go's resolveConflict, -// deliberately), which is what fix wave item 1 (Critical) is about. +// deliberately). func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, string) { t.Helper() h := sandbox(t) @@ -1097,21 +1085,19 @@ func overwriteFixture(t *testing.T) (string, *Engine, *DirPlan, *journal.Writer, return h, e, dp, j, journal.NewRunID(time.Now()) } -// TestApplyUndoReversesOverwriteRoundTrip is fix wave item 1 (CRITICAL): the -// end-to-end reproduction of the defect the final-plan review found - -// `krino undo` could not reverse a run that used (on-conflict overwrite) at -// all, by construction. reverseStep's planning-time occupancy check judged -// the displace reversal against the world exactly as it stood before any -// reversal had run, while the move-back that frees the contested path is -// ordered to execute first (reversal is last-original-step-first), so the -// displace reversal was refused every time and, being file-affecting, -// aborted the whole file's reversal - including the otherwise-safe -// move-back. This is the first coverage of undo-displace anywhere in the -// repo (grep undo-displace across every prior test returns nothing), and it -// is built from a REAL forward run through overwriteFixture's real -// displacing apply, per the brief: a hand-assembled journal.Entry is -// exactly what would let a narrower, wrong fix pass while still being -// wrong. +// TestApplyUndoReversesOverwriteRoundTrip: `krino undo` could not reverse a +// run that used (on-conflict overwrite) at all, by construction. +// reverseStep's planning-time occupancy check judged the displace reversal +// against the world exactly as it stood before any reversal had run, while +// the move-back that frees the contested path is ordered to execute first +// (reversal is last-original-step-first), so the displace reversal was +// refused every time and, being file-affecting, aborted the whole file's +// reversal - including the otherwise-safe move-back. This is the first +// coverage of undo-displace anywhere in the repo (grep undo-displace +// across every prior test returns nothing), and it is built from a REAL +// forward run through overwriteFixture's real displacing apply: a +// hand-assembled journal.Entry is exactly what would let a narrower, +// wrong fix pass while still being wrong. func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { h, e, dp, j, run := overwriteFixture(t) if _, err := e.Apply(context.Background(), dp, map[string]bool{"incoming.pdf": true}, j, run); err != nil { @@ -1170,10 +1156,10 @@ func TestApplyUndoReversesOverwriteRoundTrip(t *testing.T) { } } -// TestApplyUndoStillRefusesGenuineOccupant is fix wave item 1's second -// required test: the projection must only excuse a path an earlier step of -// THIS SAME chain is about to vacate, never turn every occupancy refusal -// into a pass. Here something outside the chain entirely - not the +// TestApplyUndoStillRefusesGenuineOccupant: the projection must only excuse +// a path an earlier step of THIS SAME chain is about to vacate, never turn +// every occupancy refusal into a pass. Here something outside the chain +// entirely - not the // displaced original, not the incoming file itself - now occupies the // path the move-back needs, and no step of this file's reversal will ever // free it. @@ -1204,9 +1190,9 @@ func TestApplyUndoStillRefusesGenuineOccupant(t *testing.T) { } } -// TestTallyFileCountsAnAllSkippedFileAsDeclined is fix wave item 4 / Minor -// 6: a file every one of whose steps came back "skipped" - the shape an -// approved all-skipped chain used to take - set none of ok/failed/declined +// TestTallyFileCountsAnAllSkippedFileAsDeclined: a file every one of whose +// steps came back "skipped" - the shape an approved all-skipped chain used +// to take - set none of ok/failed/declined // in tallyFile, so it fell out of the outcome tally entirely: "0 applied · // 0 failed · 0 declined" for a file the user was asked about and approved. // tallyFile must land every file it is given in exactly one bucket; nothing @@ -1240,24 +1226,22 @@ func TestTallyFileCountsMixedOutcomesOnceEach(t *testing.T) { } } -// --- 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. +// the shape that exercises the run-wide directory retry: 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 +// 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. @@ -1268,10 +1252,9 @@ func sharedDestUndoFixture(t *testing.T, dest string) (h string, e *Engine, logP 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. + // Three files that all move into ONE created destination: 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) @@ -1342,8 +1325,8 @@ func TestApplyUndoRemovesSharedDirectoryAfterEveryFileReverses(t *testing.T) { } } -// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst pins fix round 1's -// Important 1: retryDirRemovals must retry deepest path first. All three +// TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst: 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 @@ -1398,16 +1381,14 @@ func TestApplyUndoRetryRemovesNestedDirectoriesDeepestFirst(t *testing.T) { } } -// 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 +// TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone: +// 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"). +// it from what marks a run "(undone)" (this must not change whether a run +// shows as (undone); the test confirms that rather than assuming it). func TestApplyUndoRetryLogsBothMkdirEntriesAndStillMarksOriginalRunUndone(t *testing.T) { _, e, logPath, run := sharedDestUndoFixture(t, "Filed") diff --git a/internal/engine/engine.go b/internal/engine/engine.go index cc776e6..cde1298 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later // Package engine is what every krino front end calls: it loads and compiles -// the configuration, matches files against rules, and (from plan 3) plans -// and applies actions. It returns data; front ends only render it. +// the configuration, matches files against rules, and plans and applies +// actions. It returns data; front ends only render it. package engine import ( @@ -20,7 +20,7 @@ import ( ) // Engine holds a loaded, compiled configuration: everything a front end -// needs to check, match and (from plan 3) act. +// needs to check, match and act. type Engine struct { Config *config.Config Dirs []*Dir @@ -98,10 +98,9 @@ func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*E reported := map[string]bool{} if len(cfg.Dirs) == 0 { // With no directory to compile them for, krino.conf's excludes are - // still checked, so a mistake is reported before one is included - // (review cli F10). + // still checked, so a mistake is reported before one is included. // Compiled with the defaults' case and fold, as a directory without - // settings of its own would compile them (triage 28g). + // settings of its own would compile them. def := cfg.Main.Defaults.Over(config.Builtin()) opt := cond.Options{IgnoreCase: def.Case == config.CaseIgnore, Fold: def.Fold} for _, x := range cfg.Main.Excludes { @@ -186,7 +185,7 @@ func checkCaptures(file string, r *config.Rule, c *cond.Cond) *config.Diag { for _, a := range r.Actions { if err := plan.CheckTemplate(a.Arg); err != nil { // A placeholder that could never expand is a config error, not a - // step skipped at plan time (plan 12). + // step skipped at plan time. return &config.Diag{File: file, Pos: a.Pos, Msg: fmt.Sprintf("rule %q: %v", r.Name, err)} } n, err := plan.MaxIndex(a.Arg) diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index b6b50fc..86cf9d9 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -140,10 +140,9 @@ func TestCheck(t *testing.T) { } // TestLoadRejectsUnsuppliedCaptures: a rule using {N} must be able to get it -// from its own name tests (spec 7.3) — adapted from the brief to this -// package's actual sandbox/writeConfig/Load helpers (writeConfig takes a -// main file body and a dirs map; there is no separate engineLoad, Load is -// called directly). +// from its own name tests (spec 7.3). This uses the package's actual +// sandbox/writeConfig/Load helpers (writeConfig takes a main file body and +// a dirs map; there is no separate engineLoad, Load is called directly). func TestLoadRejectsUnsuppliedCaptures(t *testing.T) { tests := []struct{ rule, want string }{ {`(rule "a" (when (type pdf)) (move "Work/{1}"))`, @@ -261,7 +260,7 @@ func TestLoadAcceptsDuplicateWithMove(t *testing.T) { // TestLoadRefusesBadPlaceholders: a copy or move destination or a rename // name whose placeholders could never expand is a config error at load, so // krino check reports it at the action's position instead of a run skipping -// the step (plan 12). +// the step. func TestLoadRefusesBadPlaceholders(t *testing.T) { for _, c := range []struct { action, want string diff --git a/internal/engine/exclude_test.go b/internal/engine/exclude_test.go index 7d9ce2a..36f1262 100644 --- a/internal/engine/exclude_test.go +++ b/internal/engine/exclude_test.go @@ -212,7 +212,7 @@ func TestExplainReportsExclusionAndSize(t *testing.T) { // TestExcludeFailsClosedOnUnreadableContent: an exclude meant to protect // files holds when its content test cannot read a file (over max-read), so -// no rule acts on a file krino could not check (review M11). +// no rule acts on a file krino could not check. func TestExcludeFailsClosedOnUnreadableContent(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"big.txt": big, "small.txt": "nothing to hide"}) @@ -252,8 +252,7 @@ func TestExcludeFailsClosedOnUnreadableContent(t *testing.T) { } // TestLoadChecksMainExcludesWithoutDirectories: a mistake in krino.conf's -// (exclude ...) is reported even before any directory is included (review -// cli F10). +// (exclude ...) is reported even before any directory is included. func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { h := sandbox(t) main := writeConfig(t, h, "(include)\n(exclude (bogus 1))\n", nil) @@ -261,8 +260,7 @@ func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { t.Error("a broken krino.conf exclude was not reported") } // Checked with the defaults' case and fold, as a directory would compile - // them (triage 28g): a keyword of a lone combining mark is empty only - // when folded. + // them: a keyword of a lone combining mark is empty only when folded. main = writeConfig(t, h, "(include)\n(defaults (fold no))\n(exclude (content \"\u0301\"))\n", nil) if _, errs := Load(main); len(errs) != 0 { t.Errorf("checked with fold on, though the defaults say no: %v", errs) @@ -272,7 +270,7 @@ func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { // TestNoTextFormatIsNoMatch: a file whose format has no text cannot contain // a keyword, so a content exclude without a type does not set it aside, and // no "content unreadable" warning is raised - while a real read failure (over -// max-read) still fails closed (re-review N2). +// max-read) still fails closed. func TestNoTextFormatIsNoMatch(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"photo.jpg": "\xff\xd8\xff\x00\x01binary", "big.txt": big}) @@ -314,8 +312,7 @@ func TestNoTextFormatIsNoMatch(t *testing.T) { // TestTextTurningBinaryIsNoText: a file with no known extension whose first // 8 KiB read as text but which holds a NUL further on - a self-extracting // installer - counts as having no text, like an image: a content exclude -// does not set it aside and there is no warning (decided after the plan 10 -// re-check). +// does not set it aside and there is no warning. func TestTextTurningBinaryIsNoText(t *testing.T) { mixed := "confidential " + strings.Repeat("x", 9000) + "\x00tail" h, _ := excludeTree(t, map[string]string{"mixed": mixed}) @@ -388,8 +385,7 @@ func partDocx(t *testing.T, path, body string) { // TestPartlyReadableDocument: a docx with an unreadable part answers a // keyword it holds in the readable part, but a keyword not found there is -// unknown: a content exclude sets the file aside, a rule warns (plan 11, -// re-review cache F3). +// unknown: a content exclude sets the file aside, a rule warns. func TestPartlyReadableDocument(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -435,7 +431,7 @@ func TestPartlyReadableDocument(t *testing.T) { // TestExplainLeavesTheCacheAlone: explain never writes the keyword cache - // not even to remove one a directory without content tests no longer uses -// (triage 28e: explain runs without the directory's lock). +// (explain runs without the directory's lock). func TestExplainLeavesTheCacheAlone(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.txt": "x"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n"}) @@ -457,7 +453,7 @@ func TestExplainLeavesTheCacheAlone(t *testing.T) { // TestPartlyReadableDocumentIsNotCached: a partial read's answers never // enter the keyword cache, so a second run with the cache still sets the -// file aside (plan 11 review M2). +// file aside. func TestPartlyReadableDocumentIsNotCached(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -485,7 +481,7 @@ func TestPartlyReadableDocumentIsNotCached(t *testing.T) { // TestExplainAgreesWithMatchOnADuplicateExclude: a (duplicate) test inside // an exclude needs the directory's other files, as one inside a rule does, -// so explain sets aside exactly the files Match does (plan 11 review M3). +// so explain sets aside exactly the files Match does. func TestExplainAgreesWithMatchOnADuplicateExclude(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.txt": "same", "b.txt": "same", "c.txt": "other"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` @@ -514,7 +510,7 @@ func TestExplainAgreesWithMatchOnADuplicateExclude(t *testing.T) { // TestNotMatchedAfterAnUnknownRuleDoesNotAct: a partly read document an // earlier content rule could not decide is not caught by a later -// (not (matched)) catch-all (plan 11 review L6). +// (not (matched)) catch-all. func TestNotMatchedAfterAnUnknownRuleDoesNotAct(t *testing.T) { h, dl := excludeTree(t, map[string]string{}) os.MkdirAll(dl, 0o755) @@ -549,7 +545,7 @@ func TestNotMatchedAfterAnUnknownRuleDoesNotAct(t *testing.T) { } // TestDuplicateExcludeFailsClosed: an exclude whose duplicate lookup fails -// holds, marked as such (plan 12). +// holds, marked as such. func TestDuplicateExcludeFailsClosed(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -579,7 +575,7 @@ func TestDuplicateExcludeFailsClosed(t *testing.T) { // TestUndecidedStopRuleStops: a (stop) rule whose condition cannot be // decided ends the search for that file, so a later rule does not act on a -// file the stop rule was written to keep (plan 12). +// file the stop rule was written to keep. func TestUndecidedStopRuleStops(t *testing.T) { big := "confidential " + strings.Repeat("x", 2048) h, dl := excludeTree(t, map[string]string{"big.txt": big, "small.txt": "nothing"}) diff --git a/internal/engine/explain_test.go b/internal/engine/explain_test.go index 83011c5..f6ade69 100644 --- a/internal/engine/explain_test.go +++ b/internal/engine/explain_test.go @@ -69,7 +69,7 @@ func TestExplainChainMatchesThePlan(t *testing.T) { // TestExplainChainOnlyWhenAsked: the command line's explain does not build // the chain (it can hash files to resolve a conflict), and a file no rule -// acts on has none at all (plan 13 review F1, F2). +// acts on has none at all. func TestExplainChainOnlyWhenAsked(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.pdf": "one", "keep-b.pdf": "two", "new.pdf": "three"}) now := time.Now() @@ -106,7 +106,7 @@ func TestExplainChainOnlyWhenAsked(t *testing.T) { // TestExplainChainIsThisFileAlone: with two files competing for one name, // the chain shows what this file alone would do; the plan is where the two -// are resolved against each other (plan 13 review F2). +// are resolved against each other. func TestExplainChainIsThisFileAlone(t *testing.T) { h, dl := excludeTree(t, map[string]string{"a.pdf": "one", "b.pdf": "two"}) main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` diff --git a/internal/engine/facts.go b/internal/engine/facts.go index ce05df4..d664b0e 100644 --- a/internal/engine/facts.go +++ b/internal/engine/facts.go @@ -66,7 +66,7 @@ func (run *matchRun) warnings() []string { } // drainDupErrors appends every duplicate index's candidate-hashing errors -// (A1: a candidate other than the file being looked up that could not be +// (a candidate other than the file being looked up that could not be // hashed) to run.warn, once matching is done and every index has seen every // Lookup it is going to see. Candidate paths are abbreviated with // xdg.Abbrev, as every other user-visible path is. @@ -80,8 +80,8 @@ func (run *matchRun) drainDupErrors() { } } -// dupWarning is the one form of a duplicate warning (triage 6): "duplicate: -// PATH: cause", the path abbreviated like every other one shown, and an OS +// dupWarning is the one form of a duplicate warning: "duplicate: PATH: +// cause", the path abbreviated like every other one shown, and an OS // error's cause without the raw path it would repeat. func dupWarning(err error) string { return "duplicate: " + dupCause(err).Error() @@ -89,8 +89,8 @@ func dupWarning(err error) string { // dupCause is a duplicate check's error as "PATH: cause", the path // shortened with xdg.Abbrev and not repeated inside the cause - a candidate -// that could not be hashed, or an OS error on the file itself (plan 11 -// review L7). It still unwraps to err. +// that could not be hashed, or an OS error on the file itself. It still +// unwraps to err. func dupCause(err error) error { path, cause := "", err var ce dup.CandidateError @@ -229,12 +229,12 @@ func (f *facts) extract(opt cond.Options, keywords []string) { switch { case errors.Is(err, extract.ErrPartial): // Partly read: the keywords found in it are answered; one not found - // is unknown (ContentContains), and nothing is cached (plan 11). + // is unknown (ContentContains), and nothing is cached. f.partialErr = err case errors.Is(err, extract.ErrUnsupported): // A format with no text cannot contain a keyword: every answer is - // no, with no warning, and the answers are cached like any other - // (re-review N2). Only a real read failure is "unreadable". + // no, with no warning, and the answers are cached like any other. + // Only a real read failure is "unreadable". text = "" case err != nil: f.contentErr = err @@ -296,7 +296,7 @@ func (f *facts) Duplicate(dirs []string) (string, bool, error) { } // The absolute path is kept for a front end that has to act on the // other copy - the window offers to keep this one instead - while the - // reason text stays as it reads best (plan 21). + // reason text stays as it reads best. f.dupOriginal = orig return displayOriginal(orig, root), true, nil } diff --git a/internal/engine/match.go b/internal/engine/match.go index 25c05e5..9943073 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -51,7 +51,7 @@ type FileMatch struct { // absolute; "" when none did. The reason text says the same thing the // way it reads best - relative to the directory when it is inside it - // which leaves a front end no way to act on the other copy, or even to - // say where it is (his report, 2026-09-17). + // say where it is. DuplicateOf string } @@ -64,7 +64,7 @@ type Result struct { Warnings []string // directory-level, sorted; e.g. "duplicate: /x/y does not exist" // Unscanned is every existing rule destination inside the root, which // the walk leaves out (spec §8.1), so -v can say its files were not - // counted (triage 4). + // counted. Unscanned []string Elapsed time.Duration } @@ -181,7 +181,7 @@ func evalFile(run *matchRun, file scan.File) FileMatch { if r.Conf.Stop { // A (stop) rule that cannot be decided ends the search too: // it may be the rule written to keep this file from the - // ones below (plan 12). + // ones below. break } } @@ -280,7 +280,7 @@ func (e *Engine) Explain(ctx context.Context, path string) (*Explanation, error) // ExplainWithChain is Explain with Explanation.Chain filled in: the steps // this file alone would get. Building them resolves conflicts against the // disk, which can read files (a copy whose target holds the same bytes), so -// the command line's explain does not ask for it (plan 13 review F1). +// the command line's explain does not ask for it. func (e *Engine) ExplainWithChain(ctx context.Context, path string) (*Explanation, error) { return e.explain(ctx, path, true) } @@ -318,8 +318,7 @@ func (e *Engine) explain(ctx context.Context, path string, withChain bool) (*Exp skip := explainSkip(d, sf, excl, now) // The directory is walked only for a duplicate test - in a rule or in an - // exclude - the one thing that needs its other files (triage 21, plan 11 - // review M3). + // exclude - the one thing that needs its other files. files := []scan.File{sf} if len(d.DupScopes) > 0 || excludesUseDuplicate(d) { files = e.filesForExplain(d, sf, excl, now) @@ -327,7 +326,7 @@ func (e *Engine) explain(ctx context.Context, path string, withChain bool) (*Exp run := newMatchRun(e, d, ctx, now, files) if len(d.ContentKeywords) > 0 { // Loaded only: openCache would remove the cache of a directory with - // no content tests, and Explain holds no lock (triage 28e). + // no content tests, and Explain holds no lock. e.openCache(run) } f := newFacts(run, sf) @@ -413,7 +412,7 @@ func (e *Engine) cacheFingerprint(d *Dir) string { // excludedBy is what x sets a file aside as, given its evaluation: its text // when it matched, its text marked "(content unreadable)" when a content // test it reached could not read the file - an exclude protects files, so -// it fails closed (review M11) - or "" when it does not hold. +// it fails closed - or "" when it does not hold. func excludedBy(x *Exclude, res cond.Result) string { switch { case res.Match: @@ -438,7 +437,7 @@ func (e *Engine) openCache(run *matchRun) []string { } if len(run.d.ContentKeywords) == 0 { // No content test left: a cache from an earlier configuration only - // holds keywords this directory no longer uses (review cache F6). + // holds keywords this directory no longer uses. if err := os.Remove(e.cacheFile(run.d)); err != nil && !os.IsNotExist(err) { return []string{"cache: " + err.Error()} } @@ -556,7 +555,7 @@ func isBusy(path string, suffixes []string) bool { // excludeDirs computes the directories Match and Explain never enter: each // rule's copy/move destination, the Trash, and the directory holding the // main config file - each kept only when it lies strictly inside d's root -// (C3: root itself is not "inside" it here - a rule cannot exclude the very +// (root itself is not "inside" it here - a rule cannot exclude the very // directory being scanned. cmd/krino/render.go's relToRoot answers a // different question, whether a destination is root or beneath it for // display purposes, and there root does count as inside; the two are each diff --git a/internal/engine/match_test.go b/internal/engine/match_test.go index 224f881..82cfd04 100644 --- a/internal/engine/match_test.go +++ b/internal/engine/match_test.go @@ -368,8 +368,7 @@ func TestMatchDrainsDupCandidateErrors(t *testing.T) { // TestDuplicateWarningsShareOneFormat: a missing extra directory and a // candidate that cannot be hashed are both reported as "duplicate: PATH: -// cause", the path abbreviated and not repeated raw inside the cause -// (triage 6). +// cause", the path abbreviated and not repeated raw inside the cause. func TestDuplicateWarningsShareOneFormat(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -419,8 +418,7 @@ func TestDuplicateWarningsShareOneFormat(t *testing.T) { // TestRuleDuplicateWarningShortensThePath: a duplicate check that fails on // the file itself is reported on the rule with the path shortened and not -// repeated raw inside the cause, like the directory-level warnings (plan 11 -// review L7). +// repeated raw inside the cause, like the directory-level warnings. func TestRuleDuplicateWarningShortensThePath(t *testing.T) { if os.Getuid() == 0 { t.Skip("root reads a chmod 000 file") @@ -464,7 +462,7 @@ func TestRuleDuplicateWarningShortensThePath(t *testing.T) { // TestFileMatchNamesTheDuplicate: a file a (duplicate) test matched comes // back with the other copy's absolute path, so a front end can say where it // is and act on it - the reason text alone says "duplicate of NAME" for a -// copy inside the directory, which reads as no place at all (plan 21). +// copy inside the directory, which reads as no place at all. func TestFileMatchNamesTheDuplicate(t *testing.T) { e, d, _ := fixture(t) res, err := e.Match(context.Background(), d) diff --git a/internal/engine/roundtrip_test.go b/internal/engine/roundtrip_test.go index e54b0b7..218e9fe 100644 --- a/internal/engine/roundtrip_test.go +++ b/internal/engine/roundtrip_test.go @@ -181,11 +181,11 @@ func TestApplyThenUndoRestoresTheTree(t *testing.T) { } } -// TestUndoReversesChainsWithinOneFile is the property test's first finding -// (plan 8, Task 9): undo judged each move and rename against the disk as it -// is now, so the first step of a chain - a rename then a move, two moves, a -// move then trash - found its destination empty, because the later step had -// already moved the file on, and the whole file was refused. +// TestUndoReversesChainsWithinOneFile is the property test's first finding: +// undo judged each move and rename against the disk as it is now, so the +// first step of a chain - a rename then a move, two moves, a move then +// trash - found its destination empty, because the later step had already +// moved the file on, and the whole file was refused. func TestUndoReversesChainsWithinOneFile(t *testing.T) { for name, rule := range map[string]string{ "rename then move": `(rule "r" (rename "r-{name}") (move "Out"))`, diff --git a/internal/engine/session.go b/internal/engine/session.go index d79c364..c2830bd 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -105,10 +105,10 @@ func (s *Session) Apply(ctx context.Context, dp *DirPlan, approved map[string]bo // FinishDirectory ends one directory of a real run: the disk is now the // truth for the next one, so the claims start again from where this run's // files have actually ended up - every directory's, not just this one's -// (spec §7.4, plan 13 review F3). A path this directory planned but did not -// apply is free again; a path it did apply stays protected from a later -// (on-conflict overwrite) for the rest of the run, even across a directory -// that applies nothing. A dry run applies nothing and keeps every claim. +// (spec §7.4). A path this directory planned but did not apply is free +// again; a path it did apply stays protected from a later (on-conflict +// overwrite) for the rest of the run, even across a directory that applies +// nothing. A dry run applies nothing and keeps every claim. func (s *Session) FinishDirectory() { if s.dry { return diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go index 98ea1ee..5cab274 100644 --- a/internal/engine/session_test.go +++ b/internal/engine/session_test.go @@ -183,8 +183,8 @@ func TestSessionLockDirsReleasesOnFailure(t *testing.T) { // TestSessionKeepsEveryAppliedDestinationClaimed: what a directory's files // ended up at stays protected for the whole run, even across a directory -// that applies nothing (plan 13 review F3): a later (on-conflict overwrite) -// takes a free name instead of trashing an earlier directory's result. +// that applies nothing: a later (on-conflict overwrite) takes a free name +// instead of trashing an earlier directory's result. func TestSessionKeepsEveryAppliedDestinationClaimed(t *testing.T) { h := sandbox(t) old := time.Now().Add(-2 * time.Hour) diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go index 01f57d8..994c802 100644 --- a/internal/engine/undo_identity_test.go +++ b/internal/engine/undo_identity_test.go @@ -86,7 +86,7 @@ func undoFileNamed(t *testing.T, up *UndoPlan, dir, name string) UndoFile { // TestUndoRefusesAReusedTrashEntry: the Trash is emptied and another file of // the same name trashed after the run; undo must not restore that file in -// place of the one the run trashed (review M2). +// place of the one the run trashed. func TestUndoRefusesAReusedTrashEntry(t *testing.T) { e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (delete))\n"}) @@ -114,7 +114,7 @@ func TestUndoRefusesAReusedTrashEntry(t *testing.T) { // TestUndoRefusesATrashEntryThatChanged: a trash entry that is no longer the // file the run put there (its size changed) is refused, like a moved file -// that changed (review M2). +// that changed. func TestUndoRefusesATrashEntryThatChanged(t *testing.T) { e, run, _, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (delete))\n"}) @@ -132,7 +132,7 @@ func TestUndoRefusesATrashEntryThatChanged(t *testing.T) { // TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart: a.pdf from dl and // a.pdf from scans, moved in one run, are two files to undo, each refused or -// restored on its own (review M7). +// restored on its own. func TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "from dl"}, "scans": {"a.pdf": "from scans"}}, @@ -172,7 +172,7 @@ func TestUndoKeepsSameNamedFilesOfTwoDirectoriesApart(t *testing.T) { } // TestUndoRechecksAtExecution: a file edited after the undo was planned (for -// example while its review was open) is not moved back (review undo F8). +// example while its review was open) is not moved back. func TestUndoRechecksAtExecution(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -203,7 +203,7 @@ func TestUndoRechecksAtExecution(t *testing.T) { // TestUndoLeavesNoDirectoriesBehind: undo passing a file back through a // directory it had already removed for another file recreates it; that -// directory must be gone again when the undo ends (review undo F6). +// directory must be gone again when the undo ends. func TestUndoLeavesNoDirectoriesBehind(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out/{mtime:%Y}\") (move \"Out\"))\n"}) @@ -227,7 +227,7 @@ func TestUndoLeavesNoDirectoriesBehind(t *testing.T) { // TestUndoCanBeFinishedAfterAFailure: an undo whose last reversal failed // (something took the original name) leaves the file part way back; once // the obstacle is gone, undoing the same run again offers only the step -// that is left, and finishes it (review M10). +// that is left, and finishes it. func TestUndoCanBeFinishedAfterAFailure(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -278,7 +278,7 @@ func TestUndoCanBeFinishedAfterAFailure(t *testing.T) { // TestApplyReportsAStepThatCouldNotBeLogged: when the log stops accepting // writes mid-chain, the step that already ran is named in the error - file, // action and where the file is now - so the user can find what undo cannot -// see (re-review N1). +// see. func TestApplyReportsAStepThatCouldNotBeLogged(t *testing.T) { h := sandbox(t) p := filepath.Join(h, "dl", "a.pdf") @@ -319,8 +319,7 @@ func TestApplyReportsAStepThatCouldNotBeLogged(t *testing.T) { } // TestUndoRefusesOnlyTheFileWithADamagedLine: a crash that cuts one file's -// log line refuses that file; the other files of the run are still undone -// (re-review N1). +// log line refuses that file; the other files of the run are still undone. func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -362,8 +361,7 @@ func TestUndoRefusesOnlyTheFileWithADamagedLine(t *testing.T) { // TestResumedUndoStillRefusesAChangedFile: an undo that stopped after its // first reversal must not let that reversal vouch for the file later: the -// file edited in between is refused when the undo is resumed (re-review -// undo F1). +// file edited in between is refused when the undo is resumed. func TestResumedUndoStillRefusesAChangedFile(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -405,7 +403,7 @@ func TestResumedUndoStillRefusesAChangedFile(t *testing.T) { // TestUndoDoesNotOfferOnlyADirectoryRemoval: when a directory the run made // still holds a file of the user's, the restored file's remaining -// directory removal is not offered on every later undo (re-review undo F3). +// directory removal is not offered on every later undo. func TestUndoDoesNotOfferOnlyADirectoryRemoval(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -435,8 +433,7 @@ func TestUndoDoesNotOfferOnlyADirectoryRemoval(t *testing.T) { // TestApplyLogsEachStepAsItCompletes: the first step's log entry is written // before the second step runs - observed from the clock the log asks for -// each entry's time - so a run killed mid-chain leaves what it did undoable -// (review M9). +// each entry's time - so a run killed mid-chain leaves what it did undoable. func TestApplyLogsEachStepAsItCompletes(t *testing.T) { h := sandbox(t) p := filepath.Join(h, "dl", "a.pdf") @@ -475,7 +472,7 @@ func TestApplyLogsEachStepAsItCompletes(t *testing.T) { // TestUndoRefusesATrashEntryRecordedForAnotherPath: a trash entry with the // size and mtime the run logged, whose trashinfo now names another original -// path, belongs to another file and is refused (review M2). +// path, belongs to another file and is refused. func TestUndoRefusesATrashEntryRecordedForAnotherPath(t *testing.T) { e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (delete))\n"}) @@ -495,8 +492,7 @@ func TestUndoRefusesATrashEntryRecordedForAnotherPath(t *testing.T) { // TestFinishedUndoRemovesADirectoryLeftEmpty: a directory made by one file's // chain and still holding another file is not offered on its own, but once -// that other file's reversal empties it, the undo removes it (plan 10 -// re-check R1). +// that other file's reversal empties it, the undo removes it. func TestFinishedUndoRemovesADirectoryLeftEmpty(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) @@ -544,8 +540,7 @@ func TestFinishedUndoRemovesADirectoryLeftEmpty(t *testing.T) { } // TestUndoRunWithADamagedLineIsStillAnUndo: a damaged line in an undo run's -// log does not make that run look like an ordinary one that can be undone -// (plan 10 re-check R3). +// log does not make that run look like an ordinary one that can be undone. func TestUndoRunWithADamagedLineIsStillAnUndo(t *testing.T) { e, run, _, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) @@ -575,7 +570,7 @@ func TestUndoRunWithADamagedLineIsStillAnUndo(t *testing.T) { // TestUndoOverwriteThenMove: a move that replaced an existing file and was // then moved on is undone whole - the moved file goes back, and the file it -// replaced is restored to the path the later reversal vacates (triage 34m). +// replaced is restored to the path the later reversal vacates. func TestUndoOverwriteThenMove(t *testing.T) { e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "Out/a.pdf": "old"}}, map[string]string{"dl": "(path \"~/dl\")\n(ignore \"Out/\")\n(recursive yes)\n(rule \"r\" (on-conflict overwrite) (move \"Out\") (move \"Out2\"))\n"}) @@ -608,8 +603,8 @@ func TestUndoOverwriteThenMove(t *testing.T) { // TestProjectionSeesAPathAnEarlierStepWillFill: a path not on disk yet that // a queued reversal will put a file at is occupied for the steps after it, -// and free again once a later one moves that file on (triage 34m: the -// "occupied" half of the projection had no test that could fail). +// and free again once a later one moves that file on (the "occupied" half +// of the projection had no test that could fail). func TestProjectionSeesAPathAnEarlierStepWillFill(t *testing.T) { x := filepath.Join(t.TempDir(), "x.pdf") p := newUndoProjection() diff --git a/internal/extract/extract.go b/internal/extract/extract.go index c0f0262..f8f98df 100644 --- a/internal/extract/extract.go +++ b/internal/extract/extract.go @@ -29,7 +29,7 @@ var ( // ErrPartial wraps the error of a document read only in part (an // archive entry that would not open or parse). Text returns the text it // did read along with it: a keyword found there is found, but one not - // found may be in the part that could not be read (plan 11). + // found may be in the part that could not be read. ErrPartial = errors.New("part of it could not be read") // ErrTooLarge is returned when the file is larger than the configured // max-read; nothing is read in that case. @@ -70,15 +70,15 @@ var plainExt = map[string]bool{ } // zipExt is the set of Office/OpenDocument/ebook formats: a zip container -// plus XML inside it (Task 5). +// plus XML inside it. var zipExt = map[string]bool{ "docx": true, "xlsx": true, "pptx": true, "odt": true, "ods": true, "odp": true, "epub": true, } -// toolExt maps an extension to the external tool it needs (Task 6). pdf is -// handled separately, since it has its own fixed command line. +// toolExt maps an extension to the external tool it needs. pdf is handled +// separately, since it has its own fixed command line. var toolExt = map[string]string{ "doc": "antiword", // falls back to catdoc "xls": "xls2csv", @@ -108,8 +108,8 @@ func newWithPath(path string) *Extractor { for _, name := range toolNames { for _, dir := range dirs { // A relative entry would find a tool relative to the working - // directory - a bin/pdftotext an unpacked download left behind - // (review planapply F7) - so only absolute entries count. + // directory - a bin/pdftotext an unpacked download left behind - + // so only absolute entries count. if dir == "" || !filepath.IsAbs(dir) { continue } diff --git a/internal/extract/plain.go b/internal/extract/plain.go index beffae1..956c3e6 100644 --- a/internal/extract/plain.go +++ b/internal/extract/plain.go @@ -39,13 +39,12 @@ func readDecoded(path string) (string, error) { // sample that merely looks like UTF-8 must hold for the WHOLE file — no // NUL byte anywhere, and no invalid UTF-8 anywhere past the sample — or // the file is ErrUnsupported after all (a self-extracting installer has no -// text in krino's sense, decided after the plan 10 re-check); the -// Latin-1 fallback in decode -// never applies to a sniffed file, only to a file whose extension already -// names it as text. D2: when the file continues past the sample (n == -// sniffSize), the validity check is run against a trimmed copy with any -// incomplete trailing rune removed, so a multi-byte rune that happens to -// straddle byte sniffSize does not make an otherwise-valid file sniff as +// text in krino's sense); the Latin-1 fallback in decode never applies to +// a sniffed file, only to a file whose extension already names it as +// text. When the file continues past the sample (n == sniffSize), the +// validity check is run against a trimmed copy with any incomplete +// trailing rune removed, so a multi-byte rune that happens to straddle +// byte sniffSize does not make an otherwise-valid file sniff as // unsupported; sample itself, used below to build the returned text, is // left untouched — the rest of the file (read after the check) supplies // the bytes trimming set aside. @@ -89,13 +88,13 @@ func sniffText(path string) (string, error) { } // trimIncompleteTrailingRune drops an incomplete UTF-8 sequence left -// dangling at the very end of b — D2's fix for a rune cut off exactly at -// the sniff sample's boundary. It looks back at most utf8.UTFMax-1 bytes -// for the start of the trailing rune; if the bytes from there to the end -// are not a complete encoding (utf8.FullRune), that partial rune is cut, -// since more bytes to finish it may simply not have been read yet. A -// sample already ending cleanly (the common case, and every all-ASCII -// sample) is returned unchanged. +// dangling at the very end of b, so a rune cut off exactly at the sniff +// sample's boundary is not mistaken for invalid UTF-8. It looks back at +// most utf8.UTFMax-1 bytes for the start of the trailing rune; if the +// bytes from there to the end are not a complete encoding (utf8.FullRune), +// that partial rune is cut, since more bytes to finish it may simply not +// have been read yet. A sample already ending cleanly (the common case, +// and every all-ASCII sample) is returned unchanged. func trimIncompleteTrailingRune(b []byte) []byte { end := len(b) start := end - 1 @@ -143,7 +142,7 @@ func decodeUTF16(b []byte, order binary.ByteOrder) string { // decodeLatin1 decodes b as Latin-1: each byte is its own Unicode code // point. Built directly as UTF-8 (at most two bytes per input byte), not -// through a []rune of four bytes per input byte (triage 28d). +// through a []rune of four bytes per input byte. func decodeLatin1(b []byte) string { var s strings.Builder s.Grow(len(b) * 2) @@ -200,10 +199,10 @@ func stripMarkup(s string) string { gt := strings.IndexByte(s[j:], '>') if gt == -1 { - // D1: an unterminated tag (no closing '>') can no longer be - // parsed as markup, but that is no reason to discard the rest - // of the file - copy it through as literal text instead of - // simply stopping the scan. + // An unterminated tag (no closing '>') can no longer be + // parsed as markup, but that is no reason to discard the + // rest of the file - copy it through as literal text + // instead of simply stopping the scan. b.WriteString(s[i:]) break } diff --git a/internal/extract/plain_test.go b/internal/extract/plain_test.go index e53ac73..9e8b747 100644 --- a/internal/extract/plain_test.go +++ b/internal/extract/plain_test.go @@ -156,7 +156,7 @@ func TestToolsListedInOrder(t *testing.T) { // sample — sniffText must reject the whole file, not just decode what the // sample alone promised (it must not fall back to Latin-1 the way a known // text extension would). Such a file - a self-extracting installer, say - -// has no text in krino's sense (decided after the plan 10 re-check). +// has no text in krino's sense. func TestSniffWholeFileMustBeValid(t *testing.T) { e := newWithPath("") data := append([]byte(strings.Repeat("x", 8192)), 0xFF, 0x00) @@ -194,7 +194,7 @@ func TestToolLookupSkipsNonRegular(t *testing.T) { } } -// TestUnterminatedTagKeepsRemainder: D1. An unterminated ordinary tag (no +// TestUnterminatedTagKeepsRemainder: an unterminated ordinary tag (no // closing '>') must not discard the rest of the file - only the malformed // tag markup itself is unrecoverable; whatever follows it is still real // content and must still reach the extracted text. @@ -213,10 +213,10 @@ func TestUnterminatedTagKeepsRemainder(t *testing.T) { } } -// TestSniffRuneStraddlingSampleBoundary: D2. A multi-byte rune ("ż", two -// UTF-8 bytes) placed exactly so its lead byte is the sniff sample's last -// byte and its continuation byte falls just past it must not make an -// otherwise valid UTF-8 file sniff as unsupported. +// TestSniffRuneStraddlingSampleBoundary: a multi-byte rune ("ż", two UTF-8 +// bytes) placed exactly so its lead byte is the sniff sample's last byte +// and its continuation byte falls just past it must not make an otherwise +// valid UTF-8 file sniff as unsupported. func TestSniffRuneStraddlingSampleBoundary(t *testing.T) { e := newWithPath("") prefix := strings.Repeat("a", sniffSize-1) @@ -232,7 +232,7 @@ func TestSniffRuneStraddlingSampleBoundary(t *testing.T) { // TestLatin1DecodingMemory: decoding Latin-1 allocates about what the UTF-8 // result needs (at most two bytes per input byte), not a four-byte rune per -// input byte on top of it (triage 28d). +// input byte on top of it. func TestLatin1DecodingMemory(t *testing.T) { data := bytes.Repeat([]byte("Gr\xfc\xdfe "), 16000) r := testing.Benchmark(func(b *testing.B) { diff --git a/internal/extract/zipxml_test.go b/internal/extract/zipxml_test.go index 30a4cf9..5b7ba54 100644 --- a/internal/extract/zipxml_test.go +++ b/internal/extract/zipxml_test.go @@ -128,10 +128,9 @@ func TestOdfEmbeddedObjectIncluded(t *testing.T) { // The footer's mismatched end tag (</w:xyz> where </w:ftr> was open) // genuinely fails to parse under Strict=false — unlike a merely missing // end tag, which the decoder synthesises and swallows — but only after -// "broken" and the newline for </w:p> have already been written; the -// XML decoder itself confirms this token by token (see fix round 2's -// report for the trace): CharData "broken", EndElement p, then the -// error "unexpected end element </xyz>". +// "broken" and the newline for </w:p> have already been written; the XML +// decoder itself confirms this token by token: CharData "broken", +// EndElement p, then the error "unexpected end element </xyz>". func TestZipLenientOnMalformedEntry(t *testing.T) { e := newWithPath("") got, err := text(t, e, zipFile(t, "partial.docx", map[string]string{ @@ -139,7 +138,7 @@ func TestZipLenientOnMalformedEntry(t *testing.T) { "word/footer1.xml": `<w:ftr xmlns:w="w"><w:p><w:r><w:t>broken</w:t></w:r></w:p></w:xyz>`, }), 0) // Partly read: the text is kept, and ErrPartial says some of it is - // missing, so a keyword not found in it is unknown (plan 11). + // missing, so a keyword not found in it is unknown. if !errors.Is(err, ErrPartial) { t.Fatalf("good entry alongside a malformed one: err %v, want ErrPartial", err) } @@ -193,9 +192,9 @@ func TestZipNoTextAndFailingSiblingErrors(t *testing.T) { } } -// TestMaxReadCapsZipOutput: B1. A directory's max-read, when smaller than -// the fixed zipBudget default, caps a single archive's extracted text on -// its own — zipBudget is left at its default, so only budget(zipBudget, +// TestMaxReadCapsZipOutput: a directory's max-read, when smaller than the +// fixed zipBudget default, caps a single archive's extracted text on its +// own — zipBudget is left at its default, so only budget(zipBudget, // maxRead) picking the smaller maxRead explains the result. func TestMaxReadCapsZipOutput(t *testing.T) { e := newWithPath("") diff --git a/internal/journal/journal.go b/internal/journal/journal.go index 085c88a..60dc37f 100644 --- a/internal/journal/journal.go +++ b/internal/journal/journal.go @@ -92,12 +92,12 @@ func endWithNewline(f *os.File) error { // Time and ModTime are formatted with RFC3339Nano, not RFC3339: spec §9 // asks for "RFC 3339 with offset", which RFC3339Nano still is (it only adds // an optional fractional-second field; a zero-nanosecond time formats -// identically under both). Task 5's undo needs the fractional seconds: a -// refusal check comparing a file's current mtime against the mtime this -// line records must not be fooled by a file rewritten within the same -// whole second. read.go's parser already accepts fractional seconds under -// either constant (a documented time.Parse special case for RFC3339), so -// only this side needed to change. +// identically under both). Undo needs the fractional seconds: a refusal +// check comparing a file's current mtime against the mtime this line +// records must not be fooled by a file rewritten within the same whole +// second. read.go's parser already accepts fractional seconds under either +// constant (a documented time.Parse special case for RFC3339), so only +// this side needed to change. func (w *Writer) Append(e Entry) error { line := strings.Join([]string{ e.Time.Format(time.RFC3339Nano), diff --git a/internal/journal/journal_test.go b/internal/journal/journal_test.go index e8b18ae..1d53941 100644 --- a/internal/journal/journal_test.go +++ b/internal/journal/journal_test.go @@ -88,16 +88,15 @@ func TestRoundTripsAwkwardNames(t *testing.T) { } } -// TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds is item 13, promoted -// to before-commit by the plan 4 final review: nothing anywhere pinned the -// journal's on-disk time format - RFC3339Nano appears in no test file, and -// every timestamp assertion round-trips through krino's own Writer and -// Entries, so a change to something no other tool could parse would pass -// silently. The journal is the only record undo has. This reads the RAW -// bytes of a written line - not Entries, which would launder the format -// through krino's own parser - and asserts column 1 parses as RFC 3339 with -// a real numeric offset (not just "Z"), and that a time carrying -// nanoseconds keeps them. +// TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds pins the journal's +// on-disk time format: nothing else does - RFC3339Nano appears in no test +// file, and every timestamp assertion round-trips through krino's own +// Writer and Entries, so a change to something no other tool could parse +// would pass silently. The journal is the only record undo has. This +// reads the RAW bytes of a written line - not Entries, which would +// launder the format through krino's own parser - and asserts column 1 +// parses as RFC 3339 with a real numeric offset (not just "Z"), and that a +// time carrying nanoseconds keeps them. func TestAppendWritesRFC3339WithOffsetAndKeepsNanoseconds(t *testing.T) { path := filepath.Join(t.TempDir(), "state", "krino.log") w, err := Open(path) @@ -151,7 +150,7 @@ func TestAppendIsAppendOnly(t *testing.T) { // TestOpenRepairsAMissingFinalNewline: a crash can leave the log's last // line without its newline; the next run's first line must not be glued -// onto it, or that run could never be undone (review M8). +// onto it, or that run could never be undone. func TestOpenRepairsAMissingFinalNewline(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") if err := os.WriteFile(path, []byte("2026-09-11T10:02:03+02:00\tR0\tdl\ta.pdf\t1\tmo"), 0o644); err != nil { diff --git a/internal/journal/read.go b/internal/journal/read.go index 5a1de04..aa79118 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -25,12 +25,12 @@ const wantFields = 13 const undoOfPrefix = "undo of " // UndoOf returns the Detail value an undo run's run-start entry carries to -// record which run it reverses (see undoOfPrefix). Fix wave item 2 / -// final-wave item 17: before this, internal/engine wrote the same text as -// a bare string literal with nothing tying it to undoOfPrefix, so a typo in -// either would silently break Runs' Undone marking while every test stayed -// green. This is the one place that string is built; internal/engine calls -// it rather than keeping its own copy. +// record which run it reverses (see undoOfPrefix). Before this, +// internal/engine wrote the same text as a bare string literal with +// nothing tying it to undoOfPrefix, so a typo in either would silently +// break Runs' Undone marking while every test stayed green. This is the +// one place that string is built; internal/engine calls it rather than +// keeping its own copy. func UndoOf(run string) string { return undoOfPrefix + run } @@ -44,8 +44,8 @@ type Run struct { Counts map[string]int // action -> count of status "ok" Undone bool // a later run reversed this one // PartlyUndone is set with Undone while fewer of the run's reversible - // steps have been reversed, over all its undo runs, than it took - // (triage 34l): some were declined, refused or failed. + // steps have been reversed, over all its undo runs, than it took: some + // were declined, refused or failed. PartlyUndone bool UndoOf string // for an undo run, the run it reverses; "" otherwise } @@ -67,10 +67,10 @@ type ReversedKey struct { // ReversedSteps counts, for runID, every reversal that earlier undo runs of // it completed ("ok" undo- entries of runs whose run-start says they undo -// runID), so a later undo of the same run can offer only what is left -// (review M10). An undo run's own unparsable lines are skipped; a missing -// reversal is then offered again, where its own checks refuse it if it had -// in fact happened. +// runID), so a later undo of the same run can offer only what is left. An +// undo run's own unparsable lines are skipped; a missing reversal is then +// offered again, where its own checks refuse it if it had in fact +// happened. func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { lines, err := readLines(path) if err != nil { @@ -93,19 +93,20 @@ func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { return out, nil } -// Entries returns every entry belonging to runID, in file order. Since plan -// 10 (re-review N1), an unparsable line whose run column names another run -// is ignored, and one of this run whose directory and file columns are still -// readable is returned as a "damaged" entry for that file, so undo refuses -// that file alone. Otherwise a line that fails to parse is skipped, but -// Entries fails closed within the run's own window - from its run-start line to its run-end line, or to end of -// file when there is no run-end (a crashed run, which is precisely when -// corruption is likely): any unparsable line found inside that window sets -// the returned error, whether or not the line's own Run column can still be -// read back. The mere possibility that it belonged to this run is enough, -// because an incomplete chain must refuse the whole run rather than let an -// undo reverse it partway (spec §10). A line outside the window is ignored -// even when unparsable, since it cannot belong to this run. +// Entries returns every entry belonging to runID, in file order. An +// unparsable line whose run column names another run is ignored, and one +// of this run whose directory and file columns are still readable is +// returned as a "damaged" entry for that file, so undo refuses that file +// alone. Otherwise a line that fails to parse is skipped, but Entries +// fails closed within the run's own window - from its run-start line to +// its run-end line, or to end of file when there is no run-end (a crashed +// run, which is precisely when corruption is likely): any unparsable line +// found inside that window sets the returned error, whether or not the +// line's own Run column can still be read back. The mere possibility that +// it belonged to this run is enough, because an incomplete chain must +// refuse the whole run rather than let an undo reverse it partway (spec +// §10). A line outside the window is ignored even when unparsable, since +// it cannot belong to this run. // // The residual risk this leaves is a false refusal, not a false success: // krino's lock is per directory, not global, so two processes could in @@ -154,7 +155,7 @@ func Entries(path, runID string) ([]Entry, error) { run, runFound := runFieldOf(line) if runFound && run != runID { // Another run's damaged line: runs of different directories can - // interleave, and it says nothing about this one (re-review N1). + // interleave, and it says nothing about this one. continue } ours := inWindow || (runFound && run == runID) @@ -165,7 +166,7 @@ func Entries(path, runID string) ([]Entry, error) { // A line of this run cut or damaged where its file is still // readable: that file's chain may be missing a step, so it is // returned as damaged and PlanUndo refuses just that file; the - // rest of the run stays undoable (re-review N1). + // rest of the run stays undoable. out = append(out, Entry{Run: runID, Dir: dir, File: file, Action: "damaged", Status: "damaged", Detail: fmt.Sprintf("line %d", i+1)}) continue } @@ -185,8 +186,8 @@ func Entries(path, runID string) ([]Entry, error) { // runFieldOf best-effort extracts a line's Run column even when the line // otherwise fails to parse, so Entries can tell whether an unparsable line // belonged to the run it was asked for. The column counts only when a tab -// ends it: a line cut inside it holds a prefix of some run's ID, which names -// no run (plan 10 re-check R2). +// ends it: a line cut inside it holds a prefix of some run's ID, which +// names no run. func runFieldOf(line string) (string, bool) { f := strings.SplitN(line, "\t", 3) if len(f) < 3 { @@ -214,13 +215,13 @@ func fileFieldsOf(line string) (dir, file string, ok bool) { // // A run is marked Undone when a later run's run-start entry's Detail is // undoOfPrefix followed by this run's ID, AND that later run actually -// reversed something (fix wave item 2): a fully declined undo - every file -// the reviewer chose not to reverse - still opens with that same run-start -// (ApplyUndo logs a declined file exactly as spec §9 asks the forward path -// to), so the Detail alone is not proof anything happened. Reproduced by -// the reviewer: `krino undo` with every file declined left `krino log` -// reporting the original run "(undone)" regardless. What actually happened -// is provable from the same file: at least one "ok" undo-* entry. +// reversed something: a fully declined undo - every file declined rather +// than reversed - still opens with that same run-start (ApplyUndo logs a +// declined file exactly as spec §9 asks the forward path to), so the +// Detail alone is not proof anything happened. Without this check, `krino +// undo` with every file declined would leave `krino log` reporting the +// original run "(undone)" regardless. What actually happened is provable +// from the same file: at least one "ok" undo-* entry. func Runs(path string, n int) ([]Run, error) { lines, err := readLines(path) if err != nil { @@ -230,9 +231,8 @@ func Runs(path string, n int) ([]Run, error) { order := make([]string, 0) byID := make(map[string]*Run) pendingUndo := make(map[string]string) // undo run ID -> the run ID it claims to undo - // A file whose chain ended in a permanent delete is never undone, so its - // reversible steps do not count toward what a run took (plan 11 review - // L8). + // A file whose chain ended in a permanent delete is never undone, so + // its reversible steps do not count toward what a run took. type fileOf struct{ run, dir, file string } reversibleOf := map[fileOf]int{} deletedFile := map[fileOf]bool{} @@ -312,22 +312,22 @@ func Runs(path string, n int) ([]Run, error) { return runs, nil } -// ranAnyUndoStep reports whether counts - a run's own tally of "ok" actions, -// by action name - includes at least one undo- action that actually -// restored something, as opposed to merely having been started and then -// declining every file (Important 2), or having failed to restore anything -// while a wholly unrelated undo-mkdir still happened to succeed (the -// coordinator's tightening of that same fix): "undo-mkdir" is deliberately -// excluded, the one undo- action package journal cannot help but name -// directly (this package must not import internal/engine to reuse its -// isFileAffecting predicate - journal is the lower layer), but which draws -// exactly the same line that predicate does. Removing a directory once it -// turns out empty is tidiness, not a restoration: a file's own chain stops -// after a failed file-affecting reversal, but a failed or refused -// undo-mkdir never stops anything (see internal/engine's isFileAffecting -// and undoFile), so it can succeed for one file while every file-affecting -// reversal in the whole run failed - and marking the original run Undone -// from that alone would be Important 2's bug again, by a narrower route. +// ranAnyUndoStep reports whether counts - a run's own tally of "ok" +// actions, by action name - includes at least one undo- action that +// actually restored something, as opposed to merely having been started +// and then declining every file, or having failed to restore anything +// while a wholly unrelated undo-mkdir still happened to succeed: +// "undo-mkdir" is deliberately excluded, the one undo- action package +// journal cannot help but name directly (this package must not import +// internal/engine to reuse its isFileAffecting predicate - journal is the +// lower layer), but which draws exactly the same line that predicate +// does. Removing a directory once it turns out empty is tidiness, not a +// restoration: a file's own chain stops after a failed file-affecting +// reversal, but a failed or refused undo-mkdir never stops anything (see +// internal/engine's isFileAffecting and undoFile), so it can succeed for +// one file while every file-affecting reversal in the whole run failed - +// and marking the original run Undone from that alone would be the same +// bug again, by a narrower route. func ranAnyUndoStep(counts map[string]int) bool { for action, n := range counts { if n > 0 && action != "undo-mkdir" && strings.HasPrefix(action, "undo-") { diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index 4f1c902..d2bbd3a 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -104,17 +104,17 @@ func TestRunsMarksAnUndoneRun(t *testing.T) { } } -// TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined is fix wave item 2 -// (Important) / final-wave item 17: an undo run's run-start Detail alone -// used to be enough for Runs to mark the original run Undone, even when the -// undo run went on to decline every file (spec §9's "declined files are -// logged even though nothing happens to them", extended to undo) and -// reversed nothing at all. Reproduced by the reviewer via pty: `krino log` -// told the user a run had been undone when the file was still filed. Run B -// here carries the same run-start Detail as TestRunsMarksAnUndoneRun's, but -// every one of its file-scoped entries is "declined", never "ok" - the -// shape ApplyUndo logs when the front end's own review declines everything -// - so run A must come back exactly as untouched. +// TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined: an undo run's +// run-start Detail alone is not enough for Runs to mark the original run +// Undone, even when the undo run went on to decline every file (spec §9's +// "declined files are logged even though nothing happens to them", +// extended to undo) and reversed nothing at all - otherwise `krino log` +// would tell the user a run had been undone when the file was still +// filed. Run B here carries the same run-start Detail as +// TestRunsMarksAnUndoneRun's, but every one of its file-scoped entries is +// "declined", never "ok" - the shape ApplyUndo logs when the front end's +// own review declines everything - so run A must come back exactly as +// untouched. func TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -146,19 +146,19 @@ func TestRunsDoesNotMarkUndoneWhenEveryFileWasDeclined(t *testing.T) { } } -// TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir is the coordinator's -// tightening of fix wave item 2: "at least one ok undo-* entry" is still -// too loose, by the same shape as the bug it fixes. A file's own chain -// stops after a failed file-affecting reversal, but a failed or refused -// undo-mkdir deliberately does not stop anything (internal/engine's -// isFileAffecting draws exactly this line, and undoFile's stop-on-failure -// check shares it) - so an undo-mkdir belonging to one file can still -// succeed even though every file-affecting reversal in the whole run -// failed. Here x.pdf's own undo-move fails, y.pdf's own undo-move also -// fails, and z.pdf's undo-mkdir - tidying up a directory that turned out -// empty, not restoring anything - is the run's only "ok" entry. Marking -// the original run Undone from that alone would be exactly Important 2's -// bug again, by a narrower route. +// TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir: "at least one ok +// undo-* entry" alone is too loose a check, by the same shape as the bug +// it fixes above. A file's own chain stops after a failed file-affecting +// reversal, but a failed or refused undo-mkdir deliberately does not stop +// anything (internal/engine's isFileAffecting draws exactly this line, +// and undoFile's stop-on-failure check shares it) - so an undo-mkdir +// belonging to one file can still succeed even though every +// file-affecting reversal in the whole run failed. Here x.pdf's own +// undo-move fails, y.pdf's own undo-move also fails, and z.pdf's +// undo-mkdir - tidying up a directory that turned out empty, not +// restoring anything - is the run's only "ok" entry. Marking the original +// run Undone from that alone would be the same bug again, by a narrower +// route. func TestRunsDoesNotMarkUndoneWhenOnlyOkEntryIsMkdir(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -196,12 +196,12 @@ 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. +// TestEntriesCrashedRunReturnsNilError: 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 deliberate. func TestEntriesCrashedRunReturnsNilError(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -227,10 +227,10 @@ func TestEntriesCrashedRunReturnsNilError(t *testing.T) { } } -// 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. +// TestEntriesIntactRunReturnsNilError: 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) @@ -258,9 +258,9 @@ func TestEntriesIntactRunReturnsNilError(t *testing.T) { } } -// 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 - +// TestEntriesBothFailureModesReportsBadLineFirst: 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 @@ -303,9 +303,9 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { t.Fatal(err) } - // Since plan 10 a damaged line whose file is readable refuses only that - // file (a "damaged" entry); the run as a whole still fails closed here, - // on its missing run-start. + // A damaged line whose file is readable refuses only that file (a + // "damaged" entry); the run as a whole still fails closed here, on its + // missing run-start. got, err := Entries(path, "A") if err == nil { t.Fatal("Entries returned no error with a missing run-start") @@ -318,11 +318,10 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { } } -// 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: +// TestEntriesAdjacentRunStartsOneCorrupted pins what journal.Entries does +// for two runs whose run-start lines are adjacent, one of them corrupted - +// it asserts the observed behaviour, not an invented "correct" result. +// The log here is exactly: // // 1 run-start A (good) // 2 run-start B (corrupted: no tabs, unattributable) @@ -342,11 +341,10 @@ func TestEntriesBothFailureModesReportsBadLineFirst(t *testing.T) { // 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. +// This is a known inconsistency, left as is rather than fixed: 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) @@ -415,10 +413,10 @@ func TestEntriesAdjacentRunStartsOneCorrupted(t *testing.T) { // 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. Since plan 10 (re-review -// N1) a line whose directory and file columns are readable is returned as a -// "damaged" entry for that file, so only that file is refused and the rest -// of the run can still be undone. +// - PlanUndo needs to know a step went missing. A line whose directory and +// file columns are readable is returned as a "damaged" entry for that +// file, so only that file is refused and the rest of the run can still be +// undone. func TestEntriesReportsAMangledLine(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -464,9 +462,9 @@ func TestEntriesReportsAMangledLine(t *testing.T) { } } -// TestEntriesIgnoresAnotherRunsDamagedLine: a damaged line whose run column -// names another run - two directories' runs can interleave in one log - does -// not refuse this run, even inside its window (re-review N1). +// TestEntriesIgnoresAnotherRunsDamagedLine: a damaged line whose run +// column names another run - two directories' runs can interleave in one +// log - does not refuse this run, even inside its window. func TestEntriesIgnoresAnotherRunsDamagedLine(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -657,8 +655,8 @@ func TestEntriesFailsClosedOnMissingRunStart(t *testing.T) { // TestReversedStepsCountsEveryUndoOfARun: the steps earlier undo runs of a // run already reversed - only "ok" undo entries of runs that undo it - so -// a later undo of the same run can offer just what is left (review M10). -// Runs also names the run an undo run reversed. +// a later undo of the same run can offer just what is left. Runs also +// names the run an undo run reversed. func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, err := Open(path) @@ -705,8 +703,8 @@ func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) { // TestEntriesRefusesALineCutInsideItsRunColumn: a crash that cuts the last // line inside its run column leaves a prefix of some run's ID - it cannot -// be called another run's line, so inside this run's window it refuses the -// run (plan 10 re-check R2). +// be called another run's line, so inside this run's window it refuses +// the run. func TestEntriesRefusesALineCutInsideItsRunColumn(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -733,9 +731,9 @@ func TestEntriesRefusesALineCutInsideItsRunColumn(t *testing.T) { } // TestRunsMarksAPartlyUndoneRun: a run whose undo reversed some of its -// reversible steps but not all is partly undone; once a later undo reverses -// the rest, it is undone in full. A permanent delete counts toward neither -// (triage 34l). +// reversible steps but not all is partly undone; once a later undo +// reverses the rest, it is undone in full. A permanent delete counts +// toward neither. func TestRunsMarksAPartlyUndoneRun(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) @@ -780,9 +778,9 @@ func TestRunsMarksAPartlyUndoneRun(t *testing.T) { } } -// TestRunsIgnoresAPermanentlyDeletedFilesSteps: a file whose chain ended in -// a permanent delete can never be undone, so its earlier steps do not keep -// the run partly undone forever (plan 11 review L8). +// TestRunsIgnoresAPermanentlyDeletedFilesSteps: a file whose chain ended +// in a permanent delete can never be undone, so its earlier steps do not +// keep the run partly undone forever. func TestRunsIgnoresAPermanentlyDeletedFilesSteps(t *testing.T) { path := filepath.Join(t.TempDir(), "krino.log") w, _ := Open(path) diff --git a/internal/lock/lock.go b/internal/lock/lock.go index 2b98227..76a2b71 100644 --- a/internal/lock/lock.go +++ b/internal/lock/lock.go @@ -48,11 +48,10 @@ type Lock struct { // already held, so a cron job never piles up behind a stuck run. When wait // is true, Acquire polls every 100ms, with no fixed timeout - but it does // not poll forever regardless of ctx: a cancelled or expired ctx makes a -// waiting Acquire return ctx.Err() promptly instead of ignoring it (fix -// round 2026-09-12/item 3 - a run blocked waiting for a held lock must -// still notice Ctrl-C). ctx is not consulted at all when wait is false or -// the lock is free on the first try, so -y's non-waiting callers are -// unaffected. +// waiting Acquire return ctx.Err() promptly instead of ignoring it - a run +// blocked waiting for a held lock must still notice Ctrl-C. ctx is not +// consulted at all when wait is false or the lock is free on the first +// try, so -y's non-waiting callers are unaffected. // // A lock naming a pid that is not running is stale — the machine may have // lost power mid-run. Acquire removes a stale lock and retries the O_EXCL diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go index 53ce5ca..04023aa 100644 --- a/internal/lock/lock_test.go +++ b/internal/lock/lock_test.go @@ -69,12 +69,12 @@ func TestAcquireWaitsUntilReleased(t *testing.T) { } } -// TestAcquireRespectsContextCancellation is fix round 2026-09-12/item 3: a -// waiting Acquire must not ignore an interrupt - a cancelled ctx must return -// promptly with ctx.Err(), not poll forever. The unfixed code HANGS rather -// than fails here, so the wait for Acquire's result is itself bounded with -// its own hard timeout: a regression must fail this test, not hang the -// whole suite. +// TestAcquireRespectsContextCancellation: a waiting Acquire must not +// ignore an interrupt - a cancelled ctx must return promptly with +// ctx.Err(), not poll forever. A regression here would HANG rather than +// fail, so the wait for Acquire's result is itself bounded with its own +// hard timeout: a regression must fail this test, not hang the whole +// suite. func TestAcquireRespectsContextCancellation(t *testing.T) { path := filepath.Join(t.TempDir(), "dl.lock") held, err := Acquire(context.Background(), path, false) diff --git a/internal/norm/norm.go b/internal/norm/norm.go index 1c2f61d..9a22e62 100644 --- a/internal/norm/norm.go +++ b/internal/norm/norm.go @@ -20,7 +20,7 @@ const Version = 1 // Fingerprint names everything Text and Name's output depends on: Version, // and the Unicode tables of the standard library and of x/text's -// normalisation, which a Go or x/text upgrade can change (review cache F4). +// normalisation, which a Go or x/text upgrade can change. func Fingerprint() string { return fmt.Sprintf("norm%d unicode%s nfd%s", Version, unicode.Version, unorm.Version) } @@ -125,7 +125,7 @@ func FoldMapped(s string) Folded { // Source returns the original text that Text[a:b] was folded from, widened // to whole characters and to the characters right after them that fold to // nothing - the combining marks of a decomposed name, which belong to the -// letter before them (plan 11 review L5); without a map, Text[a:b] itself. +// letter before them; without a map, Text[a:b] itself. func (f Folded) Source(a, b int) string { switch { case f.same || f.start == nil: diff --git a/internal/norm/norm_test.go b/internal/norm/norm_test.go index dfae915..9d4e214 100644 --- a/internal/norm/norm_test.go +++ b/internal/norm/norm_test.go @@ -76,8 +76,8 @@ func TestFoldASCIINoAlloc(t *testing.T) { // TestFoldMappedSource: a part of the folded text leads back to the original // characters it came from, widened to whole characters and to the combining -// marks that follow them (a decomposed name, plan 11 review L5) - so a -// capture can be written with its diacritics. +// marks that follow them (a decomposed name) - so a capture can be written +// with its diacritics. func TestFoldMappedSource(t *testing.T) { cases := []struct { in, folded string diff --git a/internal/plan/chain_test.go b/internal/plan/chain_test.go index 28c208f..0c89987 100644 --- a/internal/plan/chain_test.go +++ b/internal/plan/chain_test.go @@ -118,9 +118,9 @@ func TestBuildRenameWithSlash(t *testing.T) { } } -// TestBuildKeepsSteplessChains is D6: Build returns one Chain per Input even -// when a file's rules contribute no actions, so a caller can tell "matched a -// rule that does nothing" (an exclusion) from "not matched at all". Plan 3's +// TestBuildKeepsSteplessChains: Build returns one Chain per Input even +// when a file's rules contribute no actions, so a caller can tell "matched +// a rule that does nothing" (an exclusion) from "not matched at all". The // "to act on" count and the JSON document's empty steps array both rest on // this. func TestBuildKeepsSteplessChains(t *testing.T) { diff --git a/internal/plan/conflict.go b/internal/plan/conflict.go index c660a8b..7cebc23 100644 --- a/internal/plan/conflict.go +++ b/internal/plan/conflict.go @@ -62,13 +62,13 @@ type claimed map[string]bool // the step must not run) and Displaces (non-empty only for overwrite of a // file that exists on disk). func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, c claimed) (resolved, skip, displaces string) { - // A1/A2: the file is already where this step would put it, so its own - // existence must not read as a conflict with itself. Without this guard a - // move or rename plans a rename to stem_1 and every later run adds - // another generation; under (on-conflict overwrite) the step records the - // file as its own Displaces, which plan 4 would trash before moving from - // a path that no longer exists. Checked before the policy switch, so - // overwrite never reaches its own branch. + // The file is already where this step would put it, so its own + // existence must not read as a conflict with itself. Without this + // guard a move or rename plans a rename to stem_1 and every later run + // adds another generation; under (on-conflict overwrite) the step + // records the file as its own Displaces, which would then be trashed + // before moving from a path that no longer exists. Checked before the + // policy switch, so overwrite never reaches its own branch. if dst == src { return dst, "already there", "" } @@ -81,9 +81,9 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, // through to the ordinary conflict policy below instead of failing // outright. A wrong "different" verdict costs at worst an // unnecessary suffixed copy, never data loss, so resolving the - // conflict anyway is an acceptable trade-off here (D8) - a caller - // that wants the failure itself visible would need it surfaced as - // a chain warning instead. + // conflict anyway is an acceptable trade-off here - a caller that + // wants the failure itself visible would need it surfaced as a + // chain warning instead. if same, err := d.SameContent(src, dst); err == nil && same { return dst, "already there", "" } @@ -98,17 +98,17 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, return dst, "target exists", "" case config.ConflictOverwrite: if onDisk && !c[dst] { - // Only a regular file is ever trashed to make room (review M4): - // a directory or link of the same name stays, and so does the + // Only a regular file is ever trashed to make room: a + // directory or link of the same name stays, and so does the // step - skipped, saying why. if !d.Regular(dst) { return dst, "target is not a regular file", "" } - // The existing file is trashed first (plan 4). Only the first - // step to reach this path may displace it: once another step - // in this same plan has already claimed dst, that path will - // hold that step's own output by the time this one runs, so - // displacing it again would destroy it. + // The existing file is trashed first. Only the first step to + // reach this path may displace it: once another step in this + // same plan has already claimed dst, that path will hold that + // step's own output by the time this one runs, so displacing + // it again would destroy it. return dst, "", dst } // Either claimed in-plan only (nothing on disk to displace — an @@ -124,21 +124,20 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk, resolved, skip := suffixed(dst, d, c) return resolved, skip, "" } - // Every policy has its own branch (review cli F13): a new one must not - // quietly plan as another. + // Every policy has its own branch: a new one must not quietly plan as + // another. panic(fmt.Sprintf("plan: unknown config.Conflict %d", int(policy))) } // maxSuffixAttempts bounds suffixed(): it is unbounded by design and -// terminates on a real filesystem, but C2 - without a cap, a Disk that -// always reports existence (or a directory A1 had been filling before its -// fix) turns planning quadratic instead of failing fast. +// terminates on a real filesystem, but without a cap, a Disk that always +// reports existence (or a directory being filled without the "already +// there" guard above) turns planning quadratic instead of failing fast. const maxSuffixAttempts = 10000 // suffixed finds the first stem_N.ext (N starting at 1) that is free: // neither on disk nor already claimed by an earlier step in this plan. It -// gives up after maxSuffixAttempts, returning a Skip reason and no path -// (C2). +// gives up after maxSuffixAttempts, returning a Skip reason and no path. func suffixed(dst string, d Disk, c claimed) (resolved, skip string) { dir, base := filepath.Split(dst) stem, ext := splitExt(base) diff --git a/internal/plan/placeholder_test.go b/internal/plan/placeholder_test.go index e155430..531a51f 100644 --- a/internal/plan/placeholder_test.go +++ b/internal/plan/placeholder_test.go @@ -30,7 +30,7 @@ func TestExpand(t *testing.T) { {"{mtime:%j}", "227"}, {"{now:%Y-%m-%d}", "2026-09-12"}, {"{1}", "2026"}, - // D2: an expanded value that itself contains "}}" must reach the + // An expanded value that itself contains "}}" must reach the // output unchanged. Expand's single-pass scanner jumps past a // placeholder's closing brace, so written bytes are never re-scanned; // a refactor to scan-then-replace would silently re-collapse them. @@ -77,11 +77,11 @@ func TestExpandErrors(t *testing.T) { {"{name", "unclosed placeholder"}, {"{mtime:%Q}", "unknown time format %Q in {mtime:...}"}, {"{mtime}", "unknown placeholder {mtime}"}, - // B3: the error must name the placeholder actually written ("now"), + // The error must name the placeholder actually written ("now"), // not hardcode "mtime" - the {mtime:%Q} case above passes either // way, which is why that defect survived. {"{now:%Q}", "unknown time format %Q in {now:...}"}, - // D3: {1}...{9} is the syntax (spec §7.3); {10} and up must be + // {1}...{9} is the syntax (spec §7.3); {10} and up must be // rejected the same way an unknown placeholder is. {"{10}", "unknown placeholder {10}"}, } diff --git a/internal/plan/step.go b/internal/plan/step.go index e4f280f..b3e34c1 100644 --- a/internal/plan/step.go +++ b/internal/plan/step.go @@ -20,7 +20,7 @@ const ( DeletePermanent // (delete permanent) ) -// String is for display only; Task 5's JSON representation defines its own +// String is for display only; the JSON representation defines its own // action names. func (k Kind) String() string { switch k { diff --git a/internal/trash/trash_test.go b/internal/trash/trash_test.go index 9c3eed2..38552bb 100644 --- a/internal/trash/trash_test.go +++ b/internal/trash/trash_test.go @@ -174,8 +174,8 @@ func TestPutRefusesOtherFilesystem(t *testing.T) { } // TestPutSkipsOrphanedFiles: a name already taken in files/ - left there -// without its trashinfo - is not overwritten; Put takes the next free name -// (review planapply F5). +// without its trashinfo - is not overwritten; Put takes the next free +// name. func TestPutSkipsOrphanedFiles(t *testing.T) { h := sandbox(t) orphan := filepath.Join(Dir(), "files", "a.pdf") @@ -195,8 +195,8 @@ func TestPutSkipsOrphanedFiles(t *testing.T) { } // TestPutTrashesLongNames: a name near the 255-byte limit still goes to the -// Trash - its entry name is shortened so ".trashinfo" and a suffix fit - and -// comes back under its full original name (review planapply F6). +// Trash - its entry name is shortened so ".trashinfo" and a suffix fit - +// and comes back under its full original name. func TestPutTrashesLongNames(t *testing.T) { h := sandbox(t) name := strings.Repeat("ż", 123) + ".pdf" // 246 + 4 = 250 bytes |
