diff options
Diffstat (limited to 'internal/journal/read.go')
| -rw-r--r-- | internal/journal/read.go | 110 |
1 files changed, 55 insertions, 55 deletions
diff --git a/internal/journal/read.go b/internal/journal/read.go index 5a1de04..aa79118 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -25,12 +25,12 @@ const wantFields = 13 const undoOfPrefix = "undo of " // UndoOf returns the Detail value an undo run's run-start entry carries to -// record which run it reverses (see undoOfPrefix). Fix wave item 2 / -// final-wave item 17: before this, internal/engine wrote the same text as -// a bare string literal with nothing tying it to undoOfPrefix, so a typo in -// either would silently break Runs' Undone marking while every test stayed -// green. This is the one place that string is built; internal/engine calls -// it rather than keeping its own copy. +// record which run it reverses (see undoOfPrefix). Before this, +// internal/engine wrote the same text as a bare string literal with +// nothing tying it to undoOfPrefix, so a typo in either would silently +// break Runs' Undone marking while every test stayed green. This is the +// one place that string is built; internal/engine calls it rather than +// keeping its own copy. func UndoOf(run string) string { return undoOfPrefix + run } @@ -44,8 +44,8 @@ type Run struct { Counts map[string]int // action -> count of status "ok" Undone bool // a later run reversed this one // PartlyUndone is set with Undone while fewer of the run's reversible - // steps have been reversed, over all its undo runs, than it took - // (triage 34l): some were declined, refused or failed. + // steps have been reversed, over all its undo runs, than it took: some + // were declined, refused or failed. PartlyUndone bool UndoOf string // for an undo run, the run it reverses; "" otherwise } @@ -67,10 +67,10 @@ type ReversedKey struct { // ReversedSteps counts, for runID, every reversal that earlier undo runs of // it completed ("ok" undo- entries of runs whose run-start says they undo -// runID), so a later undo of the same run can offer only what is left -// (review M10). An undo run's own unparsable lines are skipped; a missing -// reversal is then offered again, where its own checks refuse it if it had -// in fact happened. +// runID), so a later undo of the same run can offer only what is left. An +// undo run's own unparsable lines are skipped; a missing reversal is then +// offered again, where its own checks refuse it if it had in fact +// happened. func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { lines, err := readLines(path) if err != nil { @@ -93,19 +93,20 @@ func ReversedSteps(path, runID string) (map[ReversedKey]int, error) { return out, nil } -// Entries returns every entry belonging to runID, in file order. Since plan -// 10 (re-review N1), an unparsable line whose run column names another run -// is ignored, and one of this run whose directory and file columns are still -// readable is returned as a "damaged" entry for that file, so undo refuses -// that file alone. Otherwise a line that fails to parse is skipped, but -// Entries fails closed within the run's own window - from its run-start line to its run-end line, or to end of -// file when there is no run-end (a crashed run, which is precisely when -// corruption is likely): any unparsable line found inside that window sets -// the returned error, whether or not the line's own Run column can still be -// read back. The mere possibility that it belonged to this run is enough, -// because an incomplete chain must refuse the whole run rather than let an -// undo reverse it partway (spec §10). A line outside the window is ignored -// even when unparsable, since it cannot belong to this run. +// Entries returns every entry belonging to runID, in file order. An +// unparsable line whose run column names another run is ignored, and one +// of this run whose directory and file columns are still readable is +// returned as a "damaged" entry for that file, so undo refuses that file +// alone. Otherwise a line that fails to parse is skipped, but Entries +// fails closed within the run's own window - from its run-start line to +// its run-end line, or to end of file when there is no run-end (a crashed +// run, which is precisely when corruption is likely): any unparsable line +// found inside that window sets the returned error, whether or not the +// line's own Run column can still be read back. The mere possibility that +// it belonged to this run is enough, because an incomplete chain must +// refuse the whole run rather than let an undo reverse it partway (spec +// §10). A line outside the window is ignored even when unparsable, since +// it cannot belong to this run. // // The residual risk this leaves is a false refusal, not a false success: // krino's lock is per directory, not global, so two processes could in @@ -154,7 +155,7 @@ func Entries(path, runID string) ([]Entry, error) { run, runFound := runFieldOf(line) if runFound && run != runID { // Another run's damaged line: runs of different directories can - // interleave, and it says nothing about this one (re-review N1). + // interleave, and it says nothing about this one. continue } ours := inWindow || (runFound && run == runID) @@ -165,7 +166,7 @@ func Entries(path, runID string) ([]Entry, error) { // A line of this run cut or damaged where its file is still // readable: that file's chain may be missing a step, so it is // returned as damaged and PlanUndo refuses just that file; the - // rest of the run stays undoable (re-review N1). + // rest of the run stays undoable. out = append(out, Entry{Run: runID, Dir: dir, File: file, Action: "damaged", Status: "damaged", Detail: fmt.Sprintf("line %d", i+1)}) continue } @@ -185,8 +186,8 @@ func Entries(path, runID string) ([]Entry, error) { // runFieldOf best-effort extracts a line's Run column even when the line // otherwise fails to parse, so Entries can tell whether an unparsable line // belonged to the run it was asked for. The column counts only when a tab -// ends it: a line cut inside it holds a prefix of some run's ID, which names -// no run (plan 10 re-check R2). +// ends it: a line cut inside it holds a prefix of some run's ID, which +// names no run. func runFieldOf(line string) (string, bool) { f := strings.SplitN(line, "\t", 3) if len(f) < 3 { @@ -214,13 +215,13 @@ func fileFieldsOf(line string) (dir, file string, ok bool) { // // A run is marked Undone when a later run's run-start entry's Detail is // undoOfPrefix followed by this run's ID, AND that later run actually -// reversed something (fix wave item 2): a fully declined undo - every file -// the reviewer chose not to reverse - still opens with that same run-start -// (ApplyUndo logs a declined file exactly as spec §9 asks the forward path -// to), so the Detail alone is not proof anything happened. Reproduced by -// the reviewer: `krino undo` with every file declined left `krino log` -// reporting the original run "(undone)" regardless. What actually happened -// is provable from the same file: at least one "ok" undo-* entry. +// reversed something: a fully declined undo - every file declined rather +// than reversed - still opens with that same run-start (ApplyUndo logs a +// declined file exactly as spec §9 asks the forward path to), so the +// Detail alone is not proof anything happened. Without this check, `krino +// undo` with every file declined would leave `krino log` reporting the +// original run "(undone)" regardless. What actually happened is provable +// from the same file: at least one "ok" undo-* entry. func Runs(path string, n int) ([]Run, error) { lines, err := readLines(path) if err != nil { @@ -230,9 +231,8 @@ func Runs(path string, n int) ([]Run, error) { order := make([]string, 0) byID := make(map[string]*Run) pendingUndo := make(map[string]string) // undo run ID -> the run ID it claims to undo - // A file whose chain ended in a permanent delete is never undone, so its - // reversible steps do not count toward what a run took (plan 11 review - // L8). + // A file whose chain ended in a permanent delete is never undone, so + // its reversible steps do not count toward what a run took. type fileOf struct{ run, dir, file string } reversibleOf := map[fileOf]int{} deletedFile := map[fileOf]bool{} @@ -312,22 +312,22 @@ func Runs(path string, n int) ([]Run, error) { return runs, nil } -// ranAnyUndoStep reports whether counts - a run's own tally of "ok" actions, -// by action name - includes at least one undo- action that actually -// restored something, as opposed to merely having been started and then -// declining every file (Important 2), or having failed to restore anything -// while a wholly unrelated undo-mkdir still happened to succeed (the -// coordinator's tightening of that same fix): "undo-mkdir" is deliberately -// excluded, the one undo- action package journal cannot help but name -// directly (this package must not import internal/engine to reuse its -// isFileAffecting predicate - journal is the lower layer), but which draws -// exactly the same line that predicate does. Removing a directory once it -// turns out empty is tidiness, not a restoration: a file's own chain stops -// after a failed file-affecting reversal, but a failed or refused -// undo-mkdir never stops anything (see internal/engine's isFileAffecting -// and undoFile), so it can succeed for one file while every file-affecting -// reversal in the whole run failed - and marking the original run Undone -// from that alone would be Important 2's bug again, by a narrower route. +// ranAnyUndoStep reports whether counts - a run's own tally of "ok" +// actions, by action name - includes at least one undo- action that +// actually restored something, as opposed to merely having been started +// and then declining every file, or having failed to restore anything +// while a wholly unrelated undo-mkdir still happened to succeed: +// "undo-mkdir" is deliberately excluded, the one undo- action package +// journal cannot help but name directly (this package must not import +// internal/engine to reuse its isFileAffecting predicate - journal is the +// lower layer), but which draws exactly the same line that predicate +// does. Removing a directory once it turns out empty is tidiness, not a +// restoration: a file's own chain stops after a failed file-affecting +// reversal, but a failed or refused undo-mkdir never stops anything (see +// internal/engine's isFileAffecting and undoFile), so it can succeed for +// one file while every file-affecting reversal in the whole run failed - +// and marking the original run Undone from that alone would be the same +// bug again, by a narrower route. func ranAnyUndoStep(counts map[string]int) bool { for action, n := range counts { if n > 0 && action != "undo-mkdir" && strings.HasPrefix(action, "undo-") { |