aboutsummaryrefslogtreecommitdiff
path: root/internal/engine/apply.go
diff options
context:
space:
mode:
authorLukasz Kasprzak <lukas@labunix.xyz>2026-09-17 12:11:42 +0200
committerLukasz Kasprzak <lukas@labunix.xyz>2026-09-17 12:11:42 +0200
commitbddbd74e4a73e8e32bcf648efd1cac5655f6d0cd (patch)
tree187e6a1fb722d9ab97d2076f69f997f5d371e943 /internal/engine/apply.go
parentcd7425b81f963a948f0abe7df3f9e58e190c2b78 (diff)
downloadkrino-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/apply.go')
-rw-r--r--internal/engine/apply.go340
1 files changed, 165 insertions, 175 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