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/engine | |
| 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/engine')
| -rw-r--r-- | internal/engine/apply.go | 340 | ||||
| -rw-r--r-- | internal/engine/apply_test.go | 201 | ||||
| -rw-r--r-- | internal/engine/engine.go | 13 | ||||
| -rw-r--r-- | internal/engine/engine_test.go | 9 | ||||
| -rw-r--r-- | internal/engine/exclude_test.go | 28 | ||||
| -rw-r--r-- | internal/engine/explain_test.go | 4 | ||||
| -rw-r--r-- | internal/engine/facts.go | 18 | ||||
| -rw-r--r-- | internal/engine/match.go | 19 | ||||
| -rw-r--r-- | internal/engine/match_test.go | 8 | ||||
| -rw-r--r-- | internal/engine/roundtrip_test.go | 10 | ||||
| -rw-r--r-- | internal/engine/session.go | 8 | ||||
| -rw-r--r-- | internal/engine/session_test.go | 4 | ||||
| -rw-r--r-- | internal/engine/undo_identity_test.go | 39 |
13 files changed, 329 insertions, 372 deletions
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() |
