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 {