milestone 1 review: claims span the run, explain's chain is opt-in and its own, overrides keyed by clean path, splice and enum guards

5 files changed, 157 insertions, 10 deletions

diff --git a/internal/config/enum_test.go b/internal/config/enum_test.go
new file mode 100644
index 0000000..2e5b718
--- /dev/null
+++ b/internal/config/enum_test.go
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package config
+
+import (
+	"strings"
+	"testing"
+
+	"krino/internal/enumtest"
+)
+
+// TestEverySettingValuePrintsAsItselfInAConfig: PrintRule writes settings
+// with their String(), so every Conflict and CaseMode value must print as
+// the word a configuration uses, and parse back as that same value. A value
+// added later without its branch would otherwise print as "Conflict(3)",
+// which krino check refuses (plan 13 review F5).
+func TestEverySettingValuePrintsAsItselfInAConfig(t *testing.T) {
+	for _, c := range []struct {
+		typ, form string
+		printed   func(int) string
+		parsed    func(*Rule) string
+	}{
+		{"Conflict", "on-conflict", func(i int) string { return Conflict(i).String() },
+			func(r *Rule) string { return r.Settings.OnConflict.String() }},
+		{"CaseMode", "case", func(i int) string { return CaseMode(i).String() },
+			func(r *Rule) string { return r.Settings.Case.String() }},
+	} {
+		names, err := enumtest.Names("settings.go", c.typ)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(names) == 0 {
+			t.Fatalf("no %s values found", c.typ)
+		}
+		for i, name := range names {
+			word := c.printed(i)
+			if strings.ContainsAny(word, "()0123456789") {
+				t.Errorf("%s.String() = %q; give it a word a configuration uses", name, word)
+				continue
+			}
+			src := "(path \"/tmp\")\n(rule \"r\" (" + c.form + " " + word + ") (move \"Out\"))\n"
+			dir, errs := ParseDir("dl", "dl.conf", []byte(src))
+			if len(errs) > 0 {
+				t.Errorf("%s prints as %q, which krino check refuses: %v", name, word, errs)
+				continue
+			}
+			if got := c.parsed(dir.Rules[0]); got != word {
+				t.Errorf("%s prints as %q but parses back as %q", name, word, got)
+			}
+		}
+	}
+}
diff --git a/internal/config/load.go b/internal/config/load.go
index 3811802..14ce635 100644
--- a/internal/config/load.go
+++ b/internal/config/load.go
@@ -8,6 +8,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"sort"
 
 	"krino/internal/sexp"
 	"krino/internal/xdg"
@@ -20,15 +21,45 @@ type Config struct {
 	Dirs []*Dir
 }
 
+// unusedOverrides reports every override LoadWith never read: text meant
+// for a file this load does not touch, which would otherwise pass as
+// checked while the file on disk was read instead (plan 13 review F4).
+func unusedOverrides(over map[string][]byte, used map[string]bool, mainFile string) []*Diag {
+	var out []*Diag
+	for file := range over {
+		if !used[file] {
+			out = append(out, &Diag{File: mainFile, Msg: fmt.Sprintf("unsaved text for %s, which this configuration does not read", file)})
+		}
+	}
+	sort.Slice(out, func(i, j int) bool { return out[i].Msg < out[j].Msg })
+	return out
+}
+
 // readSource is the text of file: the caller's override when it has one,
-// else the file's own bytes.
-func readSource(overrides map[string][]byte, file string) ([]byte, error) {
-	if src, ok := overrides[file]; ok {
+// else the file's own bytes. It records which overrides were used, so
+// LoadWith can report one it never reached.
+func readSource(overrides map[string][]byte, used map[string]bool, file string) ([]byte, error) {
+	key := filepath.Clean(file)
+	if src, ok := overrides[key]; ok {
+		used[key] = true
 		return src, nil
 	}
 	return os.ReadFile(file)
 }
 
+// cleanOverrides keys the caller's overrides by cleaned path, so a file
+// named with a "." or ".." segment is still recognised as itself.
+func cleanOverrides(overrides map[string][]byte) map[string][]byte {
+	if len(overrides) == 0 {
+		return nil
+	}
+	out := make(map[string][]byte, len(overrides))
+	for file, src := range overrides {
+		out[filepath.Clean(file)] = src
+	}
+	return out
+}
+
 // DefaultFile is krino.conf in the XDG config directory.
 func DefaultFile() string {
 	return filepath.Join(xdg.ConfigHome(), "krino", "krino.conf")
@@ -52,7 +83,9 @@ func Load(mainFile string, names ...string) (*Config, []*Diag) {
 // checked exactly as a run would read it (GUI design §1.3). A nil map is
 // Load.
 func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*Config, []*Diag) {
-	src, err := readSource(overrides, mainFile)
+	over := cleanOverrides(overrides)
+	used := map[string]bool{}
+	src, err := readSource(over, used, mainFile)
 	if errors.Is(err, fs.ErrNotExist) {
 		return nil, []*Diag{{File: mainFile, Msg: "not found; create it with: krino init"}}
 	}
@@ -80,7 +113,7 @@ func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*C
 	}
 	for _, name := range want {
 		file := DirFile(mainFile, name)
-		src, err := readSource(overrides, file)
+		src, err := readSource(over, used, file)
 		if err != nil {
 			msg := err.Error()
 			if errors.Is(err, fs.ErrNotExist) {
@@ -93,7 +126,7 @@ func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*C
 		errs = append(errs, derrs...)
 		cfg.Dirs = append(cfg.Dirs, dir)
 	}
-	return cfg, errs
+	return cfg, append(errs, unusedOverrides(over, used, mainFile)...)
 }
 
 // Resolved is the settings that apply in dir: built-in, then the main
diff --git a/internal/config/load_test.go b/internal/config/load_test.go
index 13ef9d8..d522316 100644
--- a/internal/config/load_test.go
+++ b/internal/config/load_test.go
@@ -142,3 +142,37 @@ func TestLoadWithOverriddenText(t *testing.T) {
 		t.Errorf("the files on disk were read differently: %v %+v", errs, cfg.Dirs)
 	}
 }
+
+// TestLoadWithReportsAnUnusedOverride: an override whose path does not name
+// a file the load reads - a different spelling of it, or a directory not in
+// include - is reported, instead of the file on disk being read as though
+// the unsaved text were fine (plan 13 review F4).
+func TestLoadWithReportsAnUnusedOverride(t *testing.T) {
+	h := t.TempDir()
+	t.Setenv("HOME", h)
+	main := filepath.Join(h, "krino.conf")
+	os.MkdirAll(filepath.Join(h, "dirs"), 0o755)
+	os.WriteFile(main, []byte("(include \"dl\")\n"), 0o644)
+	os.WriteFile(filepath.Join(h, "dirs", "dl.conf"), []byte("(path \"/tmp\")\n"), 0o644)
+
+	broken := []byte("(path \"/tmp\")\n(rule \"BROKEN\")\n")
+	// The same file, spelled with a "." segment: the text must still be used.
+	uncleaned := filepath.Join(h, "dirs", ".", "dl.conf")
+	if _, errs := LoadWith(main, map[string][]byte{uncleaned: broken}); len(errs) == 0 {
+		t.Error("an override keyed by an uncleaned path was ignored")
+	}
+	// A file this load never reads: say so rather than pass silently.
+	other := filepath.Join(h, "dirs", "other.conf")
+	errs := diagText(func() []*Diag { _, e := LoadWith(main, map[string][]byte{other: broken}); return e }())
+	if !strings.Contains(errs, "other.conf") {
+		t.Errorf("an override for a file that is not read went unreported: %s", errs)
+	}
+}
+
+func diagText(ds []*Diag) string {
+	var b strings.Builder
+	for _, d := range ds {
+		b.WriteString(d.Error() + "\n")
+	}
+	return b.String()
+}
diff --git a/internal/config/print.go b/internal/config/print.go
index 5c3d467..a2244d0 100644
--- a/internal/config/print.go
+++ b/internal/config/print.go
@@ -3,6 +3,7 @@
 package config
 
 import (
+	"fmt"
 	"strings"
 
 	"krino/internal/sexp"
@@ -59,12 +60,17 @@ func PrintExclude(x *Exclude) string {
 
 // Splice replaces the bytes of the form between start and end - a form's
 // Pos and End - with text, and returns the new file contents. Every other
-// byte of src, comments and layout included, is kept exactly.
-func Splice(src []byte, start, end sexp.Pos, text string) []byte {
+// byte of src, comments and layout included, is kept exactly. Offsets that
+// do not belong to src - a form parsed from text that has since changed -
+// are an error, not a panic (plan 13 review F6).
+func Splice(src []byte, start, end sexp.Pos, text string) ([]byte, error) {
+	if start.Offset < 0 || end.Offset < start.Offset || end.Offset > len(src) {
+		return nil, fmt.Errorf("config: splice %d:%d is not inside %d bytes", start.Offset, end.Offset, len(src))
+	}
 	out := make([]byte, 0, len(src)-(end.Offset-start.Offset)+len(text))
 	out = append(out, src[:start.Offset]...)
 	out = append(out, text...)
-	return append(out, src[end.Offset:]...)
+	return append(out, src[end.Offset:]...), nil
 }
 
 // printAction renders one action form.
diff --git a/internal/config/print_test.go b/internal/config/print_test.go
index 9b8d479..510fdd9 100644
--- a/internal/config/print_test.go
+++ b/internal/config/print_test.go
@@ -5,6 +5,8 @@ package config
 import (
 	"strings"
 	"testing"
+
+	"krino/internal/sexp"
 )
 
 func parseOne(t *testing.T, body string) *Dir {
@@ -70,7 +72,11 @@ func TestSpliceLeavesTheRestAlone(t *testing.T) {
 	if len(errs) > 0 {
 		t.Fatal(errs)
 	}
-	got := string(Splice(src, dir.Rules[0].Pos, dir.Rules[0].End, "(rule \"a\"\n  (move \"In\"))"))
+	spliced, err := Splice(src, dir.Rules[0].Pos, dir.Rules[0].End, "(rule \"a\"\n  (move \"In\"))")
+	if err != nil {
+		t.Fatal(err)
+	}
+	got := string(spliced)
 	want := "; keep me\n(path \"/tmp\")\n(rule \"a\"\n  (move \"In\"))\n; and me\n"
 	if got != want {
 		t.Errorf("got\n%q\nwant\n%q", got, want)
@@ -79,3 +85,19 @@ func TestSpliceLeavesTheRestAlone(t *testing.T) {
 		t.Error("Splice changed the source it was given")
 	}
 }
+
+// TestSpliceRefusesOffsetsOutsideTheSource: a stale end offset - the file
+// changed since the form was parsed - is an error, not a panic (plan 13
+// review F6).
+func TestSpliceRefusesOffsetsOutsideTheSource(t *testing.T) {
+	src := []byte("(path \"/tmp\")")
+	for _, c := range []struct{ start, end int }{{0, len(src) + 2}, {8, 4}, {-1, 3}} {
+		if _, err := Splice(src, sexp.Pos{Offset: c.start}, sexp.Pos{Offset: c.end}, "x"); err == nil {
+			t.Errorf("Splice(%d, %d) did not refuse", c.start, c.end)
+		}
+	}
+	out, err := Splice(src, sexp.Pos{Offset: 0}, sexp.Pos{Offset: len(src)}, "(path \"/x\")")
+	if err != nil || string(out) != "(path \"/x\")" {
+		t.Errorf("Splice over the whole source = %q, %v", out, err)
+	}
+}