editing one form no longer takes its neighbour with it

A form's span was whole lines, which is wrong the moment two forms
share one.

Delete: "(exclude A) (exclude B)" on one line, delete the first, both
went. The dialog named one. The file still parsed, the live check said
no errors and Save lit, so an exclusion could disappear silently and
the next run would sort the files it had been protecting.

Clear a setting: the same shape, and the Settings window walked into it
itself - it writes "(defaults\n  (case ignore))", so the closing paren
of defaults sits on the setting's line, and putting that setting back
to "default" deleted the line and broke the file it had just written.
Every later change then silently reverted until Settings was reopened.
A hand-written "(defaults (case ignore) (fold yes))" lost fold the same
way, and that one still loaded, so it was saveable.

Both now take the whole line only when the line holds nothing else, and
otherwise take the form and the spaces after it. One helper, used by
the main file and by a directory's settings alike.

5 files changed, 142 insertions, 11 deletions

diff --git a/gui/internal/model/forms.go b/gui/internal/model/forms.go
index f0a3983..290bbc8 100644
--- a/gui/internal/model/forms.go
+++ b/gui/internal/model/forms.go
@@ -88,16 +88,46 @@ func (r *Rules) ReplaceForm(i int, text string) error {
 // DeleteForm removes form i and the comment lines directly above it - its
 // block (GUI design §5.1). A comment separated from the form by a blank
 // line belongs to the file, not to the form, and stays.
+//
+// A block is whole lines, which is wrong when another form shares one:
+// deleting the first of "(exclude A) (exclude B)" would take B as well,
+// silently, leaving a file that still parses and still checks clean. When
+// the block would reach another form, only the form's own text goes, with
+// the spaces that followed it.
 func (r *Rules) DeleteForm(i int) error {
-	f, err := r.form(i)
+	forms, err := r.Forms()
 	if err != nil {
 		return err
 	}
+	if i < 0 || i >= len(forms) {
+		return fmt.Errorf("model: no form %d", i)
+	}
+	f := forms[i]
 	start, end := r.block(f)
+	if r.blockTouchesAnotherForm(forms, i, start, end) {
+		start, end = f.Pos.Offset, f.End.Offset
+		for end < len(r.Text) && (r.Text[end] == ' ' || r.Text[end] == '\t') {
+			end++
+		}
+	}
 	r.Text = join(r.Text[:start], r.Text[end:])
 	return nil
 }
 
+// blockTouchesAnotherForm reports whether the span start..end covers any
+// part of a form other than forms[i].
+func (r *Rules) blockTouchesAnotherForm(forms []Form, i, start, end int) bool {
+	for j, other := range forms {
+		if j == i {
+			continue
+		}
+		if other.Pos.Offset < end && other.End.Offset > start {
+			return true
+		}
+	}
+	return false
+}
+
 // MoveForm moves form i one place up (delta -1) or down (delta 1), with its
 // comments. Moving past either end does nothing.
 func (r *Rules) MoveForm(i, delta int) error {
@@ -222,6 +252,31 @@ func (r *Rules) block(f Form) (start, end int) {
 	return start, end
 }
 
+// clearForm removes the form spanning start..end from text. It takes the
+// whole line - with the comment trailing it, and the blank it would leave
+// behind - only when the line holds nothing else. A setting written beside
+// something else, which is what the Settings window itself writes when it
+// creates "(defaults\n  (case ignore))", loses only itself: taking the line
+// took the closing paren of defaults with it and broke the file.
+func clearForm(text string, start, end int) string {
+	ls, le := lineStart(text, start), lineEnd(text, end)
+	before := strings.TrimSpace(text[ls:start])
+	after := text[end:le]
+	if i := strings.IndexByte(after, ';'); i >= 0 {
+		// A comment after it on the same line describes it, and goes too.
+		after = after[:i]
+	}
+	if before == "" && strings.TrimSpace(after) == "" {
+		return join(text[:ls], text[le:])
+	}
+	// Something else shares the line: drop the form and the spaces that
+	// followed it, and nothing more.
+	for end < len(text) && (text[end] == ' ' || text[end] == '\t') {
+		end++
+	}
+	return text[:start] + text[end:]
+}
+
 // lineStart is the offset just after the newline before at.
 func lineStart(s string, at int) int {
 	if at > len(s) {
diff --git a/gui/internal/model/forms_test.go b/gui/internal/model/forms_test.go
index efbd662..46b5820 100644
--- a/gui/internal/model/forms_test.go
+++ b/gui/internal/model/forms_test.go
@@ -238,3 +238,56 @@ func labels(forms []Form) []string {
 	}
 	return out
 }
+
+// TestDeleteFormLeavesItsNeighbourAlone: a form's block is whole lines, so
+// deleting one of two forms written on the same line took both. The dialog
+// names one; the other vanished, the file still parsed, the check said "no
+// errors" and Save lit - so an (exclude ...) could disappear silently and
+// the next run would sort the files it had been protecting.
+func TestDeleteFormLeavesItsNeighbourAlone(t *testing.T) {
+	r := openForms(t, "(path \"~/dl\")\n\n"+
+		"(exclude (name \"^keep-\")) (exclude (name \"^hold-\"))\n\n"+
+		"(rule \"rest\" (move \"Other\"))\n")
+	before, err := r.Forms()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(before) != 3 {
+		t.Fatalf("fixture has %d forms, want 3 (two excludes and one rule)", len(before))
+	}
+	if err := r.DeleteForm(0); err != nil { // the first exclude
+		t.Fatal(err)
+	}
+	if strings.Contains(r.Text, "^keep-") {
+		t.Error("the exclude that was asked for is still there")
+	}
+	if !strings.Contains(r.Text, "^hold-") {
+		t.Errorf("deleting the first exclude took the second with it:\n%s", r.Text)
+	}
+	if !strings.Contains(r.Text, `(rule "rest"`) {
+		t.Errorf("the rule is gone too:\n%s", r.Text)
+	}
+	after, err := r.Forms()
+	if err != nil {
+		t.Fatalf("the text no longer parses: %v\n%s", err, r.Text)
+	}
+	if len(after) != 2 {
+		t.Errorf("%d forms left, want 2:\n%s", len(after), r.Text)
+	}
+}
+
+// TestDeleteFormTakesTheWholeLineWhenItIsAlone: the ordinary case is
+// unchanged - a form on its own lines goes with its line, leaving no blank
+// where it was.
+func TestDeleteFormTakesTheWholeLineWhenItIsAlone(t *testing.T) {
+	r := openForms(t, "(path \"~/dl\")\n\n(exclude (name \"^keep-\"))\n\n(rule \"rest\" (move \"Other\"))\n")
+	if err := r.DeleteForm(0); err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(r.Text, "^keep-") {
+		t.Error("the exclude is still there")
+	}
+	if strings.Contains(r.Text, "\n\n\n") {
+		t.Errorf("a blank line was left where the form was:\n%q", r.Text)
+	}
+}
diff --git a/gui/internal/model/mainconf.go b/gui/internal/model/mainconf.go
index 951718f..afeac37 100644
--- a/gui/internal/model/mainconf.go
+++ b/gui/internal/model/mainconf.go
@@ -81,8 +81,7 @@ func (m *MainConf) SetSetting(head, args string) error {
 	}
 	switch {
 	case node != nil && args == "":
-		start, end := lineStart(m.Text, node.Pos.Offset), lineEnd(m.Text, node.End.Offset)
-		m.Text = join(m.Text[:start], m.Text[end:])
+		m.Text = clearForm(m.Text, node.Pos.Offset, node.End.Offset)
 	case node != nil:
 		m.Text = m.Text[:node.Pos.Offset] + "(" + head + " " + args + ")" + m.Text[node.End.Offset:]
 	case head == "log":
diff --git a/gui/internal/model/mainconf_test.go b/gui/internal/model/mainconf_test.go
index c77591c..bd7720e 100644
--- a/gui/internal/model/mainconf_test.go
+++ b/gui/internal/model/mainconf_test.go
@@ -167,3 +167,34 @@ func TestMainSettingRefusesAnUnknownHead(t *testing.T) {
 		t.Error("an unknown setting was read")
 	}
 }
+
+// TestClearingASettingKeepsTheRestOfItsLine: clearing removed whole lines,
+// so a setting sharing a line with anything else took it too. The Settings
+// window writes "(defaults\n  (case ignore))" itself - the closing paren of
+// defaults sits on the setting's line - so setting a value and putting it
+// back to "default" broke the file it had just written, and every later
+// change silently reverted.
+func TestClearingASettingKeepsTheRestOfItsLine(t *testing.T) {
+	m, _, _ := mainConf(t, "(include \"dl\")\n")
+	if err := m.SetSetting("case", "ignore"); err != nil {
+		t.Fatal(err)
+	}
+	if err := m.SetSetting("case", ""); err != nil {
+		t.Fatal(err)
+	}
+	if diags := m.Check(); len(diags) > 0 {
+		t.Errorf("the text no longer loads after set-then-clear: %v\n%s", diags[0], m.Text)
+	}
+
+	// The other half: a hand-written one-line (defaults ...).
+	m2, _, _ := mainConf(t, "(include \"dl\")\n(defaults (case ignore) (fold yes))\n")
+	if err := m2.SetSetting("case", ""); err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(m2.Text, "(fold yes)") {
+		t.Errorf("clearing (case ...) took (fold yes) from the same line:\n%s", m2.Text)
+	}
+	if diags := m2.Check(); len(diags) > 0 {
+		t.Errorf("the text no longer loads: %v\n%s", diags[0], m2.Text)
+	}
+}
diff --git a/gui/internal/model/settings.go b/gui/internal/model/settings.go
index 5763d68..6fa7156 100644
--- a/gui/internal/model/settings.go
+++ b/gui/internal/model/settings.go
@@ -51,8 +51,7 @@ func (r *Rules) SetSetting(head, args string) error {
 	}
 	switch {
 	case node != nil && args == "":
-		start, end := r.lineSpan(node)
-		r.Text = join(r.Text[:start], r.Text[end:])
+		r.Text = clearForm(r.Text, node.Pos.Offset, node.End.Offset)
 	case node != nil:
 		r.Text = r.Text[:node.Pos.Offset] + "(" + head + " " + args + ")" + r.Text[node.End.Offset:]
 	default:
@@ -118,12 +117,6 @@ func (r *Rules) headerEnd() (int, error) {
 	return at, nil
 }
 
-// lineSpan is the whole line a setting is written on, so clearing it takes
-// the comment that trails it rather than leaving it to dangle.
-func (r *Rules) lineSpan(n *sexp.Node) (start, end int) {
-	return lineStart(r.Text, n.Pos.Offset), lineEnd(r.Text, n.End.Offset)
-}
-
 // argsOf is a form's arguments exactly as the file writes them.
 func argsOf(n *sexp.Node, text string) string {
 	if len(n.Children) < 2 {