diff options
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() |