diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 12:11:42 +0200 |
| commit | bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd (patch) | |
| tree | 187e6a1fb722d9ab97d2076f69f997f5d371e943 /internal/apply | |
| parent | cd7425b81f963a948f0abe7df3f9e58e190c2b78 (diff) | |
| download | krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.tar.gz krino-bddbd74e4a73e8e32bcf648efd1cac5655f6d0cd.zip | |
comments that explain the code, not how it was written
About 340 comments cited the development process: task and plan
numbers, fix waves, rulings, reviewers, and the author in the third
person with a date. None of that exists outside the work itself, so to
a reader it pointed at nothing. Each one now states the engineering
reason it was standing in front of; where a comment was provenance and
nothing else, it is gone.
References to docs/design.md and docs/gui-design.md by section stay:
both ship with the repository. The design documents lose their
amendment diaries - CHANGELOG.md is that record - and the GUI's says
plainly that the window has gone further than the document.
Only comments changed. Every .go file was parsed and its code printed
with comments stripped, before and after: the two hashes are identical
across all 175 files.
Diffstat (limited to 'internal/apply')
| -rw-r--r-- | internal/apply/apply.go | 14 | ||||
| -rw-r--r-- | internal/apply/apply_test.go | 61 | ||||
| -rw-r--r-- | internal/apply/fs.go | 15 |
3 files changed, 44 insertions, 46 deletions
diff --git a/internal/apply/apply.go b/internal/apply/apply.go index a93aad4..d7bf71e 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -52,13 +52,13 @@ func Chain(c plan.Chain) []StepResult { // ChainLogged is Chain, calling done with each step's result as soon as // that step has run or been skipped, before the next one starts - so a // caller that logs from done never has a completed step missing from the -// log when the process dies mid-chain (review M9). An error from done stops -// the chain at once and is returned with the results so far. +// log when the process dies mid-chain. An error from done stops the chain +// at once and is returned with the results so far. // // A move or rename that had to take a free name at apply time, because its // planned destination was taken since planning, stops the chain too: every // later step was planned against the name the file did not get, and must -// not act on whatever is at that path (review M3). +// not act on whatever is at that path. // // Once ctx is cancelled (an interrupt), the step already under way finishes // and every later step is skipped as "interrupted": an interrupt stops after @@ -160,8 +160,8 @@ func runFileStep(step plan.Step) StepResult { var displacedEntry string if step.Displaces != "" { - // Re-checked at apply time (review M4): only a regular file may be - // trashed to make room, never a directory or link put there since. + // Re-checked at apply time: only a regular file may be trashed to + // make room, never a directory or link put there since. if fi, err := os.Lstat(step.Displaces); err != nil || !fi.Mode().IsRegular() { return StepResult{Step: step, Status: "failed", Detail: "the file to replace is gone or no longer a regular file"} } @@ -224,8 +224,8 @@ func runFileStep(step plan.Step) StepResult { // journal columns describe the file at Dst after the step, and after a // trash step the file genuinely lives there, so recording it is more // useful than an empty column and stays greppable. It also cannot confuse -// undo: Task 5's refusal condition for reversing a trash step is "the -// entry is gone, or Src now exists" — it reads Entry and Src, never Dst. +// undo: the refusal condition for reversing a trash step is "the entry is +// gone, or Src now exists" — it reads Entry and Src, never Dst. func runTrashStep(step plan.Step) StepResult { entry, err := trash.Put(step.Src) if err != nil { diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 4bf6c30..6134988 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -153,15 +153,15 @@ func TestChainSkippedStepIsNotAttempted(t *testing.T) { _ = time.Now } -// TestChainDisplacedFileRestoresFromDisplacedEntry is the fix-round-1 test: -// DisplacedEntry must be usable for undo, not merely present. It proves -// that by actually restoring the displaced file from the Trash and checking -// its content, not just that the field is non-empty. The displaced file -// sits at its own path, distinct from the step's own Dst: were the two the -// same (the ordinary overwrite shape), the mover's own file would already -// occupy that name by the time Restore ran, and Restore correctly refuses -// to land on an occupied path — this test isolates DisplacedEntry's own -// round-trip instead of also exercising that refusal. +// TestChainDisplacedFileRestoresFromDisplacedEntry: DisplacedEntry must be +// usable for undo, not merely present. It proves that by actually +// restoring the displaced file from the Trash and checking its content, +// not just that the field is non-empty. The displaced file sits at its own +// path, distinct from the step's own Dst: were the two the same (the +// ordinary overwrite shape), the mover's own file would already occupy +// that name by the time Restore ran, and Restore correctly refuses to land +// on an occupied path — this test isolates DisplacedEntry's own round-trip +// instead of also exercising that refusal. func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { root := t.TempDir() t.Setenv("HOME", root) @@ -205,12 +205,12 @@ func TestChainDisplacedFileRestoresFromDisplacedEntry(t *testing.T) { } } -// TestChainRunsRenameStep is the fix-round-2 gap: apply_test.go's only other -// Rename (in TestChainStopsWhenFileChanged) is always reported "skipped", -// because the Move before it is made to fail on purpose, so -// "case plan.Rename: err = os.Rename(step.Src, dst)" is never exercised by -// a passing test. A reversed-argument typo there would compile, pass every -// other test, pass make ci, and surface only as live data corruption. +// TestChainRunsRenameStep: apply_test.go's only other Rename (in +// TestChainStopsWhenFileChanged) is always reported "skipped", because the +// Move before it is made to fail on purpose, so "case plan.Rename: err = +// os.Rename(step.Src, dst)" is never exercised by a passing test. A +// reversed-argument typo there would compile, pass every other test, pass +// make ci, and surface only as live data corruption. func TestChainRunsRenameStep(t *testing.T) { root := t.TempDir() src := write(t, filepath.Join(root, "x.pdf"), "content", 0o644) @@ -229,12 +229,12 @@ func TestChainRunsRenameStep(t *testing.T) { } } -// TestChainMadeIsOutermostFirstForNestedDirectories is the fix-round-2 gap: -// every other test creates at most one missing directory level, so -// mkdirAllTracked's outermost-first ordering is correct by trace but -// unpinned by any assertion. Task 5 removes these directories in reverse, -// so a later accidental reordering would break undo while passing -// everything else here. +// TestChainMadeIsOutermostFirstForNestedDirectories: every other test +// creates at most one missing directory level, so mkdirAllTracked's +// outermost-first ordering is correct by trace but unpinned by any +// assertion. Undo removes these directories in reverse, so a later +// accidental reordering would break it while passing everything else +// here. func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { root := t.TempDir() write(t, filepath.Join(root, "x.pdf"), "content", 0o644) @@ -251,12 +251,11 @@ func TestChainMadeIsOutermostFirstForNestedDirectories(t *testing.T) { } } -// TestMoveFileRefusesOccupiedDestination is item 16 (fix round 2026-09-12, -// plan 5 Task 2): moveFile must refuse an occupied destination on its own, -// not merely rely on runFileStep having already checked - the exact -// arrangement that produced plan 4's Task 5 Critical, where a helper that -// replaced silently was trusted because some caller had checked. Called -// directly, bypassing runFileStep's own pre-check entirely. +// TestMoveFileRefusesOccupiedDestination: moveFile must refuse an occupied +// destination on its own, not merely rely on runFileStep having already +// checked - trusting that some caller had checked is exactly what let a +// silently replacing helper cause harm. Called directly, bypassing +// runFileStep's own pre-check entirely. func TestMoveFileRefusesOccupiedDestination(t *testing.T) { dir := t.TempDir() src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) @@ -273,10 +272,10 @@ func TestMoveFileRefusesOccupiedDestination(t *testing.T) { } } -// TestRenameFileRefusesOccupiedDestination is item 16's other half: -// runFileStep's bare os.Rename call for the Rename kind was just as -// unguarded in itself as moveFile was. renameFile is the helper that now -// carries the same independent guard, called directly here. +// TestRenameFileRefusesOccupiedDestination: runFileStep's bare os.Rename +// call for the Rename kind was just as unguarded in itself as moveFile +// was. renameFile is the helper that now carries the same independent +// guard, called directly here. func TestRenameFileRefusesOccupiedDestination(t *testing.T) { dir := t.TempDir() src := write(t, filepath.Join(dir, "x.pdf"), "source", 0o644) diff --git a/internal/apply/fs.go b/internal/apply/fs.go index 56737e8..a8feccc 100644 --- a/internal/apply/fs.go +++ b/internal/apply/fs.go @@ -108,13 +108,12 @@ func moveFile(src, dst string) error { if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { return err } - // Item 16 (fix round 2026-09-12, plan 5 Task 2): this guard must hold - // independently of runFileStep's own pre-check, layered rather than - // moved - the exact arrangement that produced plan 4's Task 5 Critical, - // where a helper that replaced silently was trusted because some caller - // had checked. Placed immediately before the operation that would - // otherwise clobber dst, the same way copyFile's own guard sits right - // before its rename into place. + // This guard must hold independently of runFileStep's own pre-check, + // layered rather than moved: trusting that some caller already checked + // is exactly what lets a silently replacing helper cause harm. Placed + // immediately before the operation that would otherwise clobber dst, + // the same way copyFile's own guard sits right before its rename into + // place. if err := refuseIfExists(dst); err != nil { return err } @@ -132,7 +131,7 @@ func moveFile(src, dst string) error { } // renameFile renames src to dst, refusing on its own when dst already -// exists rather than trusting that a caller checked first (item 16, same +// exists rather than trusting that a caller checked first (the same // reasoning as moveFile's guard above): a bare os.Rename silently replaces // an occupied destination, and runFileStep's own pre-check must not be the // only thing standing between a rename step and that. |
