diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 13:59:52 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-17 13:59:52 +0200 |
| commit | b44222fc2b061382dc601014cde287cc41b857ca (patch) | |
| tree | 3181b6d3336bb6766803a9fc0ab76e09198aa253 | |
| parent | 9ab6686b98499c745024a474a71e3d99b6e14973 (diff) | |
| download | krino-b44222fc2b061382dc601014cde287cc41b857ca.tar.gz krino-b44222fc2b061382dc601014cde287cc41b857ca.zip | |
a duplicate test in an exclude protects like one in a rule
The scopes that drive "no rule deletes a file krino found to be a
duplicate" were collected from rules only. A directory whose duplicate
tests lived in (exclude ...) forms had no scopes at all, so the
protection never engaged: krino explain said "yes duplicate" and the
next rule permanently deleted every copy. The README's promise was
false in that shape, and the spec's wording permitted it.
What matters is what krino knows, not which form taught it. The spec
and krino.conf(5) now say so too.
| -rw-r--r-- | docs/design.md | 10 | ||||
| -rw-r--r-- | internal/engine/engine.go | 32 | ||||
| -rw-r--r-- | internal/engine/nodelete_test.go | 34 | ||||
| -rw-r--r-- | man/krino.conf.5 | 10 |
4 files changed, 70 insertions, 16 deletions
diff --git a/docs/design.md b/docs/design.md index 3037308..6299288 100644 --- a/docs/design.md +++ b/docs/design.md @@ -295,10 +295,12 @@ jdupes. Two rules enforce this: 1. A rule whose condition contains `(duplicate)` cannot contain a `delete` action (§4.5). -2. In a directory whose rules use `(duplicate)`, a file that is a duplicate - under any of the duplicate scopes those rules use — each distinct set of - DIRs, and the plain `(duplicate)`, looked up for the file whether or not - evaluation reached that test — gets no delete step from any rule. The +2. In a directory that uses `(duplicate)` — in a rule or in an `(exclude + ...)`, since what matters is what krino knows and not which form taught + it — a file that is a duplicate under any of the duplicate scopes that + directory uses — each distinct set of DIRs, and the plain `(duplicate)`, + looked up for the file whether or not evaluation reached that test — + gets no delete step from any rule. The plan shows the step as "skipped: a duplicate is never deleted" and the rest of the chain continues from the file's current path. This covers what rule 1 cannot see: a later rule deleting through `(matched)` or through a diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 0c3186d..61c310e 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -159,7 +159,7 @@ func LoadWith(mainFile string, overrides map[string][]byte, names ...string) (*E } } dir.ContentKeywords = contentKeywords(dir.Rules, dir.Excludes) - dir.DupScopes = dupScopes(dir.Rules) + dir.DupScopes = dupScopes(dir.Rules, dir.Excludes) dirs = append(dirs, dir) } @@ -267,16 +267,24 @@ func contentKeywords(rules []*Rule, excludes []*Exclude) []cond.Keyword { return out } -// dupScopes returns the distinct Cond.DupDirs lists of rules, in first-seen -// order. Lists are compared as written, with their length in the key so -// (duplicate) and (duplicate "") stay apart; two spellings of one directory -// stay two entries, which costs a second lookup but never a wrong answer, -// since facts.Duplicate resolves and shares the index itself. -func dupScopes(rules []*Rule) [][]string { +// dupScopes returns the distinct Cond.DupDirs lists a directory uses, in +// first-seen order, from its rules and its excludes alike. Lists are +// compared as written, with their length in the key so (duplicate) and +// (duplicate "") stay apart; two spellings of one directory stay two +// entries, which costs a second lookup but never a wrong answer, since +// facts.Duplicate resolves and shares the index itself. +// +// The excludes count because the protection these scopes drive - no rule +// deletes a file krino has found to be a duplicate - is about what krino +// knows, not about which form taught it. A directory whose only duplicate +// test sat in an (exclude ...) had no scopes at all, so a later rule could +// permanently delete every copy of a file krino had just called a +// duplicate. +func dupScopes(rules []*Rule, excludes []*Exclude) [][]string { var out [][]string seen := map[string]bool{} - for _, r := range rules { - for _, dirs := range r.Cond.DupDirs { + add := func(lists [][]string) { + for _, dirs := range lists { key := fmt.Sprintf("%d\x00%s", len(dirs), strings.Join(dirs, "\x00")) if seen[key] { continue @@ -285,6 +293,12 @@ func dupScopes(rules []*Rule) [][]string { out = append(out, dirs) } } + for _, r := range rules { + add(r.Cond.DupDirs) + } + for _, x := range excludes { + add(x.Cond.DupDirs) + } return out } diff --git a/internal/engine/nodelete_test.go b/internal/engine/nodelete_test.go index af7597c..6b8ffa9 100644 --- a/internal/engine/nodelete_test.go +++ b/internal/engine/nodelete_test.go @@ -210,3 +210,37 @@ func TestNoDeleteWhenTheDuplicateCheckFails(t *testing.T) { t.Errorf("a.pdf was not deleted: %v", err) } } + +// TestDuplicateInAnExcludeStillProtects: the promise is that no rule may +// delete a file krino has found to be a duplicate under any scope its +// directory uses (README, "Safety"). The scopes were collected from rules +// only, so a directory whose duplicate tests live in (exclude ...) forms +// had no scopes at all and the protection never engaged - a later rule +// permanently deleted every copy. +func TestDuplicateInAnExcludeStillProtects(t *testing.T) { + home, _ := dlTree(t, map[string]int{"a.pdf": 1, "b.pdf": 2, "c.pdf": 3}, sameBytes) + conf := "(path \"~/dl\")\n(recursive yes)\n(min-age 0s)\n" + + "(exclude (not (duplicate)))\n" + + "(rule \"purge\" (when (type pdf)) (delete permanent))\n" + main := writeConfig(t, home, `(include "dl")`, map[string]string{"dl": conf}) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + dp, err := e.Plan(context.Background(), e.Dirs[0], plan.NewClaims()) + if err != nil { + t.Fatal(err) + } + planned := 0 + for _, c := range dp.Chains { + for _, st := range c.Steps { + if st.Kind == plan.DeletePermanent && st.Skip == "" { + planned++ + t.Errorf("%s: a duplicate is planned for permanent deletion", c.File.Rel) + } + } + } + if planned == 0 && len(dp.Chains) == 0 { + t.Skip("no chains were planned at all; the fixture does not exercise the rule") + } +} diff --git a/man/krino.conf.5 b/man/krino.conf.5 index a9b3ea8..1a60ecc 100644 --- a/man/krino.conf.5 +++ b/man/krino.conf.5 @@ -1319,9 +1319,13 @@ A rule combining .Ic (duplicate) with a delete action is refused .Pq Sx RULES . -And in a directory whose rules use -.Ic (duplicate) , -a file that is a duplicate under any of the duplicate scopes those rules use +And in a directory that uses +.Ic (duplicate) +\(em in a rule or in an +.Ic (exclude ...) , +since what matters is what krino knows and not which form taught it \(em +a file that is a duplicate under any of the duplicate scopes that directory +uses \(em each distinct set of .Ar dir arguments, and the plain |
