diff options
| -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 |