aboutsummaryrefslogtreecommitdiff
path: root/cmd/krino/sort.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 /cmd/krino/sort.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 'cmd/krino/sort.go')
-rw-r--r--cmd/krino/sort.go135
1 files changed, 66 insertions, 69 deletions
diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index c2038fd..12812c7 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -26,13 +26,12 @@ import (
// stdin is os.Stdin, threaded through this seam rather than referenced
// directly: cmdSort's terminal check, installSignalHandler and reviewDir
// all read it, and a test must never depend on what the ambient test
-// binary's stdin happens to be (fix round 2026-09-12/item 4). If it were
-// ever a real terminal, code that only worked by assuming otherwise would
-// fall through to the interactive prompt and block the test suite on a
-// keypress - the same kind of hang the lock-cancellation test was built to
-// never risk. Tests point this at something guaranteed non-terminal
-// (commands_test.go's home helper) instead of relying on a claim about
-// what go test does with stdin.
+// binary's stdin happens to be. If it were ever a real terminal, code that
+// only worked by assuming otherwise would fall through to the interactive
+// prompt and block the test suite on a keypress - the same kind of hang
+// the lock-cancellation test was built to never risk. Tests point this at
+// something guaranteed non-terminal (commands_test.go's home helper)
+// instead of relying on a claim about what go test does with stdin.
var stdin = os.Stdin
// stdinIsTerminal is the check that review can prompt at all, a seam so a
@@ -46,11 +45,11 @@ var stdinIsTerminal = func() bool { return term.IsTerminal(int(stdin.Fd())) }
// (and cannot drift) across the three places it applies.
const zeroOutcome = "0 applied · 0 failed · 0 declined"
-// cmdSort plans and, from Task 7, applies the included directories: flags
-// are checked before any config is read, a bad config stops the whole run
-// before scanning (spec §11), and one engine.Session - its log, run id and
-// claims - covers every directory in the run. See docs/design.md
-// §8.2-§8.4 and §11 for the flow this follows.
+// cmdSort plans and applies the included directories: flags are checked
+// before any config is read, a bad config stops the whole run before
+// scanning (spec §11), and one engine.Session - its log, run id and claims
+// - covers every directory in the run. See docs/design.md §8.2-§8.4 and
+// §11 for the flow this follows.
func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
if g.yes && g.dry {
return usageError(stderr, "-y and -n cannot be used together")
@@ -80,9 +79,9 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
return usageError(stderr, "refusing to prompt: stdin is not a terminal (use -y or -n)")
}
- // Ruling 5: internal/tui deliberately does not trap signals - a package
- // that installs process-wide handlers as a side effect of reading one
- // key would surprise every caller. It lands here because cmdSort must
+ // internal/tui deliberately does not trap signals - a package that
+ // installs process-wide handlers as a side effect of reading one key
+ // would surprise every caller. It lands here because cmdSort must
// install one anyway: spec §11 says Ctrl-C finishes the current step,
// logs it, and stops, which means the ctx passed to Apply below must be
// cancelled on SIGINT. The same handler also restores the terminal on
@@ -96,8 +95,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
// The run itself - the log, its run id and the claims every directory
// shares - belongs to the engine, so the GUI runs a directory exactly
- // as this does (GUI design §1.3). Ruling 2: a dry session opens no log,
- // since journal.Open creates the state directory and an empty krino.log
+ // as this does (GUI design §1.3). A dry session opens no log, since
+ // journal.Open creates the state directory and an empty krino.log
// merely by being called.
sess, err := e.NewSession(g.dry)
if err != nil {
@@ -125,9 +124,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
// Spec §3/§11: a second krino on the same directory waits for the
// lock, or fails immediately with -y, so a cron job never piles up
// behind a stuck run. lock.Acquire takes ctx precisely so that wait
- // is not unbounded in practice (fix round 2026-09-12/item 1): a
- // signal cancels it and Acquire returns ctx.Err() promptly instead
- // of polling forever.
+ // is not unbounded in practice: a signal cancels it and Acquire
+ // returns ctx.Err() promptly instead of polling forever.
l, err := sess.Lock(ctx, d, !g.yes)
if err != nil {
if interrupted(err) {
@@ -176,15 +174,15 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
}
if g.json {
// --json is only ever reached with -n (checked above), and
- // Ruling 7 is explicit that JSON must never be paged, so
- // this returns before any of the paging/review code below.
+ // JSON must never be paged, so this returns before any of
+ // the paging/review code below.
jsonDirs = append(jsonDirs, plan.NewJSONDir(d.Name, d.Root, dp.Chains, fileNotes(dp.Result), dp.Result.Warnings))
return false
}
- // Ruling 6: the plan goes through tui.Page - taller than the
- // terminal, it is shown through $PAGER and the prompt follows
- // once the pager exits (spec §8.2) - for -n as much as for the
+ // The plan goes through tui.Page - taller than the terminal, it
+ // is shown through $PAGER and the prompt follows once the
+ // pager exits (spec §8.2) - for -n as much as for the
// interactive and -y paths; a dry run that scrolls 200 files
// off the top of the terminal is exactly the case the pager
// exists for. printPlan is reused as-is (render.go), never
@@ -228,8 +226,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
case 's':
// Spec §8.2: [s] applies nothing in this directory and
// moves on - no Apply call at all, so nothing is logged
- // for it either (Ruling 1: this is a chosen outcome, not a
- // failure, and must not set exit 1).
+ // for it either: this is a chosen outcome, not a failure,
+ // and must not set exit 1.
fmt.Fprintln(stdout, zeroOutcome)
return false
case 'q':
@@ -254,11 +252,11 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
}
res, aerr := sess.Apply(ctx, toApply, approved)
if aerr != nil {
- // Interrupted mid-apply (fix round 2026-09-12/item 2):
- // treated exactly like the cancelled lock wait above - not
- // a failure ("context canceled" is a Go-ism, not something
- // to show a user who just pressed Ctrl-C). The ctx.Err()
- // check at the end of this function turns it into exit 130.
+ // Interrupted mid-apply: treated exactly like the
+ // cancelled lock wait above - not a failure ("context
+ // canceled" is a Go-ism, not something to show a user who
+ // just pressed Ctrl-C). The ctx.Err() check at the end of
+ // this function turns it into exit 130.
if !interrupted(aerr) {
fmt.Fprintf(stderr, "krino: %s: %v\n", d.Name, aerr)
exit = 1
@@ -266,8 +264,8 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
return stopAfterApply(action, aerr)
}
fmt.Fprintln(stdout, withNotReviewed(outcome(p, res.Applied, res.Failed, res.Declined), notReviewed))
- // Ruling 1: only an actual step failure makes the run exit 1
- // here - a directory the user declined or skipped must not.
+ // Only an actual step failure makes the run exit 1 here - a
+ // directory the user declined or skipped must not.
if res.Failed > 0 {
exit = 1
}
@@ -305,12 +303,12 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
}
// actionableChains returns the chains of dp.Chains that have at least one
-// step that will actually run (chainActing, render.go) - fix wave item 4 /
-// Minor 5: this used to be a separate len(c.Steps) > 0 check, which
-// disagreed with render.go's countActing over a chain every one of whose
-// steps is skipped, so a directory could report "0 to act on" and then
-// still offer such a chain for approval. Converged on chainActing, this is
-// now also stricter than the filter engine.Apply's own forward-path loop
+// step that will actually run (chainActing, render.go). This used to be a
+// separate len(c.Steps) > 0 check, which disagreed with render.go's
+// countActing over a chain every one of whose steps is skipped, so a
+// directory could report "0 to act on" and then still offer such a chain
+// for approval. Converged on chainActing, this is now also stricter than
+// the filter engine.Apply's own forward-path loop
// applies (internal/engine/apply.go's Apply, still len(c.Steps) > 0): an
// all-skipped chain is simply never a candidate for approval here, so it
// can never reach Apply with approved == true, and Apply's own loop -
@@ -328,12 +326,12 @@ func actionableChains(chains []plan.Chain) []plan.Chain {
// installSignalHandler arranges for SIGINT and SIGTERM to cancel cancel
// and, if stdin is a terminal, restore it to the state it was in when this
-// was called (Ruling 5). The returned func stops the handler and must be
-// called once the run is over, or its goroutine and signal registration
-// outlive cmdSort.
+// was called. The returned func stops the handler and must be called once
+// the run is over, or its goroutine and signal registration outlive
+// cmdSort.
//
-// Fix round 2026-09-12/item 2: the handler loops rather than servicing one
-// signal and exiting. A single-shot select left signal.Notify's
+// The handler loops rather than servicing one signal and exiting. A
+// single-shot select left signal.Notify's
// registration in place (which suppresses Go's default terminate) with no
// goroutine left reading the channel, so a second Ctrl-C landed in the
// buffered channel unread and a third was dropped outright - together with
@@ -356,8 +354,8 @@ func installSignalHandler(cancel context.CancelFunc) func() {
sig := make(chan os.Signal, 1)
signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
// A hangup stops krino like SIGTERM - unless it was started ignoring
- // hangups (nohup), which must go on meaning "keep running" (re-review
- // pa F3): asking for SIGHUP would re-enable it.
+ // hangups (nohup), which must go on meaning "keep running": asking for
+ // SIGHUP would re-enable it.
if !signal.Ignored(syscall.SIGHUP) {
signal.Notify(sig, syscall.SIGHUP)
}
@@ -374,20 +372,19 @@ func installSignalHandler(cancel context.CancelFunc) func() {
signals++
if signals >= 2 {
// Deliberate exception to "the lock is released on
- // every path" (fix round 2026-09-12/item 3, by
- // design, documented on review): every deferred
- // cleanup in cmdSort - including the held directory's
- // lock.Release - is skipped here. That is intentional:
- // this is the user's escape hatch when a clean
- // shutdown was already asked for once (the first
- // signal) and not delivered, so trying to unwind
- // cleanly a second time is exactly what would make the
- // hatch unreliable. It is safe to skip that unwind:
- // journal.Append flushes each line as it writes, so
- // nothing buffered is lost by exiting immediately, and
- // an abandoned lock file is reclaimed automatically by
- // Task 4's stale-pid takeover the next time anything
- // tries to acquire it (lock.go's tryAcquire).
+ // every path": every deferred cleanup in cmdSort -
+ // including the held directory's lock.Release - is
+ // skipped here. That is intentional: this is the
+ // user's escape hatch when a clean shutdown was
+ // already asked for once (the first signal) and not
+ // delivered, so trying to unwind cleanly a second time
+ // is exactly what would make the hatch unreliable. It
+ // is safe to skip that unwind: journal.Append flushes
+ // each line as it writes, so nothing buffered is lost
+ // by exiting immediately, and an abandoned lock file is
+ // reclaimed automatically by the stale-pid takeover
+ // the next time anything tries to acquire it (lock.go's
+ // tryAcquire).
os.Exit(130)
}
case <-done:
@@ -497,17 +494,17 @@ func printSkipped(w io.Writer, skipped []scan.Skipped) {
}
}
-// skipReasonOrder is plan 2's reviewed order for the skip reasons the last
-// line reports, before "unmatched".
+// skipReasonOrder is the order the skip reasons the last line reports,
+// before "unmatched".
var skipReasonOrder = []scan.Reason{scan.Ignored, scan.Busy, scan.TooNew, scan.TooBig, scan.Symlink, scan.NotRegular, scan.Unreadable}
// skipSummaryLine builds the "not acted on: N ignored · N busy · ... · N
// unmatched" line per spec §8.2's item format ("<count> <label>", not
// "<label>: <count>"), only the non-zero counts, or "" when every count is
-// zero. Ordering is plan 2's reviewed skipReasonOrder, with "unmatched"
-// last: the spec's own worked example shows only three of the seven
-// categories and states no ordering rule, so its incidental order is not
-// adopted, only its item format and unmatched's trailing position.
+// zero. Ordering follows skipReasonOrder, with "unmatched" last: the
+// spec's own worked example shows only three of the seven categories and
+// states no ordering rule, so its incidental order is not adopted, only
+// its item format and unmatched's trailing position.
func skipSummaryLine(r *engine.Result, chains []plan.Chain, verbose bool) string {
counts := map[scan.Reason]int{}
for _, s := range r.Skipped {
@@ -600,8 +597,8 @@ func interrupted(err error) bool {
}
// stopAfterApply reports whether krino stops once a directory has been
-// applied: an interrupt stops it, and so does [w], whether or not its apply
-// succeeded (review cli F3) - no later directory is planned or asked about.
+// applied: an interrupt stops it, and so does [w], whether or not its
+// apply succeeded - no later directory is planned or asked about.
func stopAfterApply(action rune, err error) bool {
return interrupted(err) || action == 'w'
}