diff options
| author | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-14 10:56:21 +0200 |
|---|---|---|
| committer | Lukasz Kasprzak <lukas@labunix.xyz> | 2026-09-14 10:56:21 +0200 |
| commit | 1506c7dcd6032c04c1df6f5785b6dd3dfe4511cc (patch) | |
| tree | fb6d573ca8726c1b1d91ba273a284ba666cde3f6 | |
| parent | ebdd7bb254a0f19f815a644d26ce232de7be0adb (diff) | |
| download | krino-1506c7dcd6032c04c1df6f5785b6dd3dfe4511cc.tar.gz krino-1506c7dcd6032c04c1df6f5785b6dd3dfe4511cc.zip | |
krino: duplicates are found, never deleted
A rule combining (duplicate) with a delete action is refused at load, and a
file that is a duplicate under any duplicate scope its directory uses gets no
delete step from any rule: the plan shows it skipped and the chain continues.
A failed duplicate check blocks the delete too. Tests cover (matched), another
rule's own condition, a test evaluation skipped, two scopes and a failed
check, each checking every copy is still on disk.
| -rw-r--r-- | CHANGELOG.md | 7 | ||||
| -rw-r--r-- | README.md | 4 | ||||
| -rw-r--r-- | cmd/krino/matching_test.go | 4 | ||||
| -rw-r--r-- | cmd/krino/sort_test.go | 101 | ||||
| -rw-r--r-- | docs/design.md | 4 | ||||
| -rw-r--r-- | internal/engine/engine.go | 50 | ||||
| -rw-r--r-- | internal/engine/engine_test.go | 44 | ||||
| -rw-r--r-- | internal/engine/match.go | 42 | ||||
| -rw-r--r-- | internal/engine/match_test.go | 2 | ||||
| -rw-r--r-- | internal/engine/nodelete_test.go | 212 | ||||
| -rw-r--r-- | internal/engine/plan.go | 2 | ||||
| -rw-r--r-- | internal/plan/chain.go | 8 | ||||
| -rw-r--r-- | internal/plan/chain_test.go | 28 | ||||
| -rw-r--r-- | man/krino.conf.5 | 123 |
14 files changed, 551 insertions, 80 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bd80c8..36b50e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +- Duplicates are found, never deleted. A rule combining `(duplicate)` with + `(delete)` or `(delete permanent)` is refused by `krino check` and every + run, and a file that is a duplicate under any duplicate scope its + directory's rules use gets no delete step from any rule. This replaces + 0.0.1's known limitation: duplicate conditions with different scopes can no + longer delete every copy of a group, only move copies aside. + ## 0.0.1 — 2026-09-13 - Per-directory rule files, a main file listing which directories run, and a @@ -189,6 +189,10 @@ rule content only when a private pattern list is configured with - `(delete)` moves a file to the freedesktop.org Trash by default, not `unlink(2)` — recoverable, by hand or with `krino undo`. `(delete permanent)` is the explicit opt-out, and undo can never reverse it. +- Duplicates are found, never deleted. A rule can combine `(duplicate)` with + `move`, never with `delete`, and no other rule can delete a file krino has + found to be a duplicate. Move duplicates aside, for example to `~/.dupes/`, + and delete them later yourself or with a tool such as jdupes. - Every step of every run is appended to `$XDG_STATE_HOME/krino/krino.log` (default `~/.local/state/krino/krino.log`), tab-separated and `grep`-able. diff --git a/cmd/krino/matching_test.go b/cmd/krino/matching_test.go index 69c0eca..25a2993 100644 --- a/cmd/krino/matching_test.go +++ b/cmd/krino/matching_test.go @@ -18,7 +18,7 @@ const dlRules = ` (recursive yes) (min-age 0s) (ignore "*.part") -(rule "dups" (when (duplicate)) (delete) (stop)) +(rule "dups" (when (duplicate)) (move "Dupes") (stop)) (rule "acme" (when (type document) (content "acme ltd")) (move "Work/Acme") (stop)) (rule "images" (when (type image)) (move "Pictures")) (rule "rest" (when (not (matched)) (type text)) (move "Other")) @@ -66,7 +66,7 @@ func TestDryRun(t *testing.T) { "krino: dl ~/dl\n8 scanned · 4 to act on · 2 warnings · ", "\n 1 inv1.txt move → Work/Acme/ acme type txt, content \"acme ltd\"\n", "\n 2 notes.txt move → Other/ rest not matched, type txt\n", - "\n 4 report (1).pdf trash dups duplicate of report.pdf\n", + "\n 4 report (1).pdf move → Dupes/ dups duplicate of report.pdf\n", "\nwarnings\n brochure.doc acme: content unreadable: needs antiword or catdoc, not installed\n", "\nnot acted on: 1 ignored · 1 busy · 2 unmatched (-v lists them)\n", } { diff --git a/cmd/krino/sort_test.go b/cmd/krino/sort_test.go index fa9a7ca..321c9e8 100644 --- a/cmd/krino/sort_test.go +++ b/cmd/krino/sort_test.go @@ -111,14 +111,14 @@ func TestAllSkippedDirectoryReportsZeroAndLogsNothing(t *testing.T) { } } -// TestPermanentDeleteOfDuplicatesUnderOverlappingDirKeepsACopy drives the -// whole run end to end on the shape where a (duplicate "DIR") overlaps the -// scanned tree: a recursive root holding Archive/x.pdf and a loose, older -// x-copy.pdf with the same bytes, and a rule that permanently deletes -// duplicates of anything under Archive. Whatever the verdicts, applying -// the plan must leave the content on disk; the expected outcome is that -// the archived copy stays and only the loose one goes. -func TestPermanentDeleteOfDuplicatesUnderOverlappingDirKeepsACopy(t *testing.T) { +// TestDuplicatesUnderOverlappingDirMoveOnlyTheLooseCopy drives the whole run +// end to end on the shape where a (duplicate "DIR") overlaps the scanned +// tree: a recursive root holding Archive/x.pdf and a loose, older +// x-copy.pdf with the same bytes, and a rule that moves duplicates of +// anything under Archive aside. Every lookup must elect the same original, +// so the archived copy stays where it is and only the loose one is moved; +// an inconsistent election would move both. +func TestDuplicatesUnderOverlappingDirMoveOnlyTheLooseCopy(t *testing.T) { h := home(t) dl := filepath.Join(h, "dl") content := []byte("%PDF acme statement") @@ -143,33 +143,86 @@ func TestPermanentDeleteOfDuplicatesUnderOverlappingDirKeepsACopy(t *testing.T) t.Fatal(errOut) } rules := "(path \"~/dl\")\n(recursive yes)\n(min-age 0s)\n" + - "(rule \"dups\" (when (duplicate \"Archive\")) (delete permanent))\n" + "(rule \"dups\" (when (duplicate \"Archive\")) (move \"~/dupes\") (stop))\n" if err := os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644); err != nil { t.Fatal(err) } code, out, errOut := runCLI(t, "-y") + if code != 0 { + t.Fatalf("run: exit %d\nstdout:\n%s\nstderr:\n%s", code, out, errOut) + } + moved := filepath.Join(h, "dupes", "x-copy.pdf") + for _, p := range []string{archived, moved} { + b, err := os.ReadFile(p) + if err != nil || string(b) != string(content) { + t.Errorf("%s: want the content there, got err %v", p, err) + } + } + if _, err := os.Stat(loose); !os.IsNotExist(err) { + t.Errorf("%s is still in place; want it moved to %s", loose, moved) + } +} - var copies []string - err := filepath.WalkDir(dl, func(p string, d os.DirEntry, err error) error { - if err != nil || !d.Type().IsRegular() { - return err +// TestCheckRefusesDuplicateWithDelete: krino check reports the §4.5 +// refusal on stderr and fails, before any run could act on the rule. +func TestCheckRefusesDuplicateWithDelete(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) + } + if code, _, errOut := runCLI(t, "init"); code != 0 { + t.Fatal(errOut) + } + if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 { + t.Fatal(errOut) + } + rules := "(path \"~/dl\")\n(rule \"d\" (when (duplicate)) (delete))\n" + if err := os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) + } + code, _, errOut := runCLI(t, "check") + want := `rule "d": (duplicate) cannot be combined with (delete)` + if code == 0 || !strings.Contains(errOut, want) { + t.Errorf("check: exit %d, stderr %q; want non-zero and %q", code, errOut, want) + } +} + +// TestDryRunShowsNeverDeletedSkip pins the plan line spec §5.5 names. +func TestDryRunShowsNeverDeletedSkip(t *testing.T) { + h := home(t) + dl := filepath.Join(h, "dl") + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for name, mt := range map[string]time.Time{"a.pdf": old, "b.pdf": old.Add(time.Hour)} { + p := filepath.Join(dl, name) + if err := os.MkdirAll(dl, 0o755); err != nil { + t.Fatal(err) } - if b, err := os.ReadFile(p); err == nil && string(b) == string(content) { - copies = append(copies, p) + if err := os.WriteFile(p, []byte("%PDF same"), 0o644); err != nil { + t.Fatal(err) } - return nil - }) - if err != nil { - t.Fatal(err) + if err := os.Chtimes(p, mt, mt); err != nil { + t.Fatal(err) + } + } + if code, _, errOut := runCLI(t, "init"); code != 0 { + t.Fatal(errOut) + } + if code, _, errOut := runCLI(t, "new", "dl", dl); code != 0 { + t.Fatal(errOut) } - if len(copies) == 0 { - t.Fatalf("every copy was deleted (exit %d)\nstdout:\n%s\nstderr:\n%s", code, out, errOut) + rules := "(path \"~/dl\")\n(min-age 0s)\n" + + "(rule \"dupes\" (when (duplicate)) (move \"Dupes\"))\n" + + "(rule \"cleanup\" (when (matched)) (delete))\n" + if err := os.WriteFile(filepath.Join(h, ".config", "krino", "dirs", "dl.conf"), []byte(rules), 0o644); err != nil { + t.Fatal(err) } + code, out, errOut := runCLI(t, "-n") if code != 0 { - t.Fatalf("run: %d %s", code, errOut) + t.Fatalf("exit %d: %s", code, errOut) } - if len(copies) != 1 || copies[0] != archived { - t.Errorf("copies left = %v, want only %s", copies, archived) + if !strings.Contains(out, "skipped: a duplicate is never deleted") { + t.Errorf("plan lacks the skipped delete:\n%s", out) } } diff --git a/docs/design.md b/docs/design.md index 150e135..153bab0 100644 --- a/docs/design.md +++ b/docs/design.md @@ -243,7 +243,9 @@ jdupes. Two rules enforce this: 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 - condition of its own. + condition of its own. If that lookup fails, the delete is skipped too, as + "duplicate check failed, so not deleted: REASON": krino cannot show the + file is not a duplicate, so it keeps it. Together they mean no rule can delete every copy of content that a duplicate test in that directory can see. Under each scope every scanned file in a diff --git a/internal/engine/engine.go b/internal/engine/engine.go index ea61e8c..b5f2106 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -8,6 +8,7 @@ package engine import ( "fmt" "os" + "strings" "time" "krino/internal/cond" @@ -44,6 +45,12 @@ type Dir struct { // other variant will ever be asked for; with more than one, both must // stay memoised, as before. ContentVariants []cond.Options + + // DupScopes is every distinct directory list the duplicate tests of + // Rules use, in first-seen order, the plain (duplicate) as an empty + // list. Spec §5.5 rule 2 looks a file up under each of them before any + // rule may delete it. + DupScopes [][]string } // Rule is one directory's rule, with its condition compiled. @@ -90,9 +97,14 @@ func Load(mainFile string, names ...string) (*Engine, []*config.Diag) { errs = append(errs, diag) continue } + if diag := checkDuplicateDelete(d.File, r, c); diag != nil { + errs = append(errs, diag) + continue + } dir.Rules = append(dir.Rules, &Rule{Name: r.Name, Conf: r, Settings: rs, Cond: c}) } dir.ContentVariants = contentVariants(dir.Rules) + dir.DupScopes = dupScopes(dir.Rules) dirs = append(dirs, dir) } @@ -132,6 +144,23 @@ func checkCaptures(file string, r *config.Rule, c *cond.Cond) *config.Diag { return nil } +// checkDuplicateDelete refuses a rule that combines a duplicate test with a +// delete action (spec §4.5, §5.5): duplicates are found, never deleted. +// Cond.DupDirs records every duplicate test compiled, inside or and not +// too, so a test anywhere in the condition counts. It reports only the +// first delete action, so one config mistake yields one diagnostic. +func checkDuplicateDelete(file string, r *config.Rule, c *cond.Cond) *config.Diag { + if len(c.DupDirs) == 0 { + return nil + } + for _, a := range r.Actions { + if a.Kind == config.Delete || a.Kind == config.DeletePermanent { + return &config.Diag{File: file, Pos: a.Pos, Msg: fmt.Sprintf("rule %q: (duplicate) cannot be combined with (%s): duplicates are never deleted, move them aside instead", r.Name, a.Kind)} + } + } + return nil +} + // captureGroups renders a capture-group count with correct singular/plural. func captureGroups(n int) string { if n == 1 { @@ -177,6 +206,27 @@ func contentVariants(rules []*Rule) []cond.Options { 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 { + var out [][]string + seen := map[string]bool{} + for _, r := range rules { + for _, dirs := range r.Cond.DupDirs { + key := fmt.Sprintf("%d\x00%s", len(dirs), strings.Join(dirs, "\x00")) + if seen[key] { + continue + } + seen[key] = true + out = append(out, dirs) + } + } + return out +} + // Report is what Check reports: the files involved and each directory's // state. type Report struct { diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 8dcfcb3..18b35e7 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -213,3 +213,47 @@ func TestContentVariantsComputedAtLoad(t *testing.T) { t.Fatalf("ContentVariants = %+v, want %+v", got, want) } } + +// TestLoadRefusesDuplicateWithDelete is spec §4.5: a (duplicate) test +// anywhere in a rule's condition, and a delete action in the same rule, is +// a load error, however the test is nested. +func TestLoadRefusesDuplicateWithDelete(t *testing.T) { + tests := []struct{ rule, want string }{ + {`(rule "a" (when (duplicate)) (delete))`, + `rule "a": (duplicate) cannot be combined with (delete): duplicates are never deleted, move them aside instead`}, + {`(rule "a" (when (duplicate "Archive")) (delete permanent))`, + `rule "a": (duplicate) cannot be combined with (delete permanent)`}, + {`(rule "a" (when (or (type pdf) (duplicate))) (move "Keep") (delete))`, + `rule "a": (duplicate) cannot be combined with (delete)`}, + {`(rule "a" (when (not (duplicate))) (delete))`, + `rule "a": (duplicate) cannot be combined with (delete)`}, + } + for _, tt := range tests { + h := sandbox(t) + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": `(path "/tmp") +` + tt.rule}) + _, errs := Load(main, "dl") + joined := "" + for _, d := range errs { + joined += d.Error() + "\n" + } + if len(errs) != 1 || !strings.Contains(joined, tt.want) { + t.Errorf("rule %s: errs %v, want %q", tt.rule, errs, tt.want) + } + } +} + +// TestLoadAcceptsDuplicateWithMove: the way §5.5 recommends dealing with +// duplicates loads clean, and so does a delete rule with no duplicate test. +func TestLoadAcceptsDuplicateWithMove(t *testing.T) { + for _, rule := range []string{ + `(rule "dupes" (when (duplicate "Archive")) (move "~/.dupes/") (stop))`, + `(rule "old" (when (type iso) (age > 90d)) (delete))`, + } { + h := sandbox(t) + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": "(path \"/tmp\")\n" + rule}) + if _, errs := Load(main, "dl"); len(errs) != 0 { + t.Errorf("rule %s: errs %v, want none", rule, errs) + } + } +} diff --git a/internal/engine/match.go b/internal/engine/match.go index e68a6e8..ffee7f0 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -33,6 +33,12 @@ type FileMatch struct { File scan.File Rules []RuleMatch // matching rules in order, ending at the first with (stop) Warnings []string // "<rule>: <warning>", e.g. "acme: content unreadable: needs pdftotext, not installed" + + // NoDelete is non-empty when no delete step may run for this file (spec + // §5.5 rule 2), and says why: the file is a duplicate under a scope its + // directory's rules use, or that check failed. Only set for a file some + // matching rule would delete. + NoDelete string } // Result is everything Match found in one directory. @@ -127,9 +133,45 @@ func evalFile(run *matchRun, file scan.File) FileMatch { break } } + if len(run.d.DupScopes) > 0 && deletes(fm.Rules) { + fm.NoDelete = noDelete(f, run.d.DupScopes) + } return fm } +// NeverDeleted is the reason a duplicate's delete step is skipped (spec +// §5.5). +const NeverDeleted = "a duplicate is never deleted" + +// deletes reports whether any of rules has a delete action. +func deletes(rules []RuleMatch) bool { + for _, rm := range rules { + for _, a := range rm.Rule.Conf.Actions { + if a.Kind == config.Delete || a.Kind == config.DeletePermanent { + return true + } + } + } + return false +} + +// noDelete looks f up under every scope, whether or not evaluating the +// rules reached that test (spec §5.5 rule 2), and returns why f must not be +// deleted, or "" when it may be. A failed lookup blocks the delete too: +// krino cannot show the file is not a duplicate, so it keeps it. +func noDelete(f *facts, scopes [][]string) string { + for _, dirs := range scopes { + _, dup, err := f.Duplicate(dirs) + if err != nil { + return "duplicate check failed, so not deleted: " + err.Error() + } + if dup { + return NeverDeleted + } + } + return "" +} + // RuleTrace is one rule's outcome in an Explain call. type RuleTrace struct { Rule *Rule diff --git a/internal/engine/match_test.go b/internal/engine/match_test.go index 1356074..477e06f 100644 --- a/internal/engine/match_test.go +++ b/internal/engine/match_test.go @@ -18,7 +18,7 @@ const dlConf = ` (recursive yes) (min-age 0s) (ignore "*.part") -(rule "dups" (when (duplicate)) (delete) (stop)) +(rule "dups" (when (duplicate)) (move "Dupes") (stop)) (rule "acme" (when (type document) (content "acme ltd")) (move "Work/Acme") (stop)) (rule "images" (when (type image)) (move "Pictures")) (rule "rest" (when (not (matched)) (type text)) (move "Other")) diff --git a/internal/engine/nodelete_test.go b/internal/engine/nodelete_test.go new file mode 100644 index 0000000..80d9ed9 --- /dev/null +++ b/internal/engine/nodelete_test.go @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +package engine + +import ( + "context" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + "testing" + "time" + + "krino/internal/journal" + "krino/internal/plan" +) + +// sameBytes is the content every file in these tests shares. +const sameBytes = "%PDF identical bytes" + +// dlTree creates ~/dl in a sandbox. Each file holds body and is modified +// the given number of hours after a fixed old time, so the smallest number +// is the oldest file. PATH is emptied so no extraction tool runs. +func dlTree(t *testing.T, files map[string]int, body string) (home, dl string) { + t.Helper() + home = sandbox(t) + t.Setenv("PATH", t.TempDir()) + dl = filepath.Join(home, "dl") + old := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + for rel, hours := range files { + p := filepath.Join(dl, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + mt := old.Add(time.Duration(hours) * time.Hour) + if err := os.Chtimes(p, mt, mt); err != nil { + t.Fatal(err) + } + } + return home, dl +} + +// planAndApply configures ~/dl (recursive, min-age 0s) with rules, plans +// it, approves and applies every chain, and returns the plan. +func planAndApply(t *testing.T, home, rules string) *DirPlan { + t.Helper() + conf := "(path \"~/dl\")\n(recursive yes)\n(min-age 0s)\n" + rules + 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) + } + approved := map[string]bool{} + for _, c := range dp.Chains { + approved[c.File.Rel] = true + } + j, err := journal.Open(filepath.Join(home, ".local", "state", "krino", "krino.log")) + if err != nil { + t.Fatal(err) + } + defer j.Close() + res, err := e.Apply(context.Background(), dp, approved, j, journal.NewRunID(time.Now())) + if err != nil { + t.Fatal(err) + } + if res.Failed != 0 { + t.Fatalf("%d files failed: %+v", res.Failed, res) + } + return dp +} + +// assertCopies checks that exactly the files want (paths relative to dl, +// in any order) hold body after the run. +func assertCopies(t *testing.T, dl, body string, want []string) { + t.Helper() + var got []string + err := filepath.WalkDir(dl, func(p string, d fs.DirEntry, err error) error { + if err != nil || !d.Type().IsRegular() { + return err + } + if b, err := os.ReadFile(p); err == nil && string(b) == body { + rel, _ := filepath.Rel(dl, p) + got = append(got, filepath.ToSlash(rel)) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + sort.Strings(got) + w := append([]string(nil), want...) + sort.Strings(w) + if strings.Join(got, "\n") != strings.Join(w, "\n") { + t.Errorf("copies on disk = %q, want %q", got, w) + } +} + +// assertSkip checks that rel's chain has a step from rule skipped with a +// reason starting with want. +func assertSkip(t *testing.T, dp *DirPlan, rel, rule, want string) { + t.Helper() + for _, c := range dp.Chains { + if c.File.Rel != rel { + continue + } + for _, s := range c.Steps { + if s.Rule == rule && strings.HasPrefix(s.Skip, want) { + return + } + } + t.Errorf("%s: no step of rule %q skipped with %q; steps %+v", rel, rule, want, c.Steps) + return + } + t.Errorf("%s: no chain in the plan", rel) +} + +// TestNoDeleteThroughMatched: a later rule deleting through (matched) +// cannot delete what an earlier duplicate rule found. +func TestNoDeleteThroughMatched(t *testing.T) { + h, dl := dlTree(t, map[string]int{"a.pdf": 0, "b.pdf": 1}, sameBytes) + dp := planAndApply(t, h, ` +(rule "dupes" (when (duplicate)) (move "Dupes")) +(rule "cleanup" (when (matched)) (delete permanent)) +`) + assertCopies(t, dl, sameBytes, []string{"a.pdf", "Dupes/b.pdf"}) + assertSkip(t, dp, "b.pdf", "cleanup", NeverDeleted) +} + +// TestNoDeleteThroughAnotherRulesCondition: a rule with no duplicate test +// of its own deletes the original, but not the duplicate. +func TestNoDeleteThroughAnotherRulesCondition(t *testing.T) { + h, dl := dlTree(t, map[string]int{"a.pdf": 0, "b.pdf": 1}, sameBytes) + dp := planAndApply(t, h, ` +(rule "pdfs" (when (type pdf)) (delete permanent)) +(rule "dupes" (when (duplicate)) (move "Dupes")) +`) + assertCopies(t, dl, sameBytes, []string{"Dupes/b.pdf"}) + assertSkip(t, dp, "b.pdf", "pdfs", NeverDeleted) +} + +// TestNoDeleteWhenEvaluationSkippedTheDuplicateTest: "dupes" tests size +// first, which is false, so its duplicate test is never evaluated; b.pdf +// is still a duplicate under that scope, so "pdfs" cannot delete it. +func TestNoDeleteWhenEvaluationSkippedTheDuplicateTest(t *testing.T) { + h, dl := dlTree(t, map[string]int{"a.pdf": 0, "b.pdf": 1}, sameBytes) + dp := planAndApply(t, h, ` +(rule "dupes" (when (size > 1G) (duplicate)) (move "Dupes")) +(rule "pdfs" (when (type pdf)) (delete permanent)) +`) + assertCopies(t, dl, sameBytes, []string{"b.pdf"}) + assertSkip(t, dp, "b.pdf", "pdfs", NeverDeleted) +} + +// TestNoDeleteWithTwoScopesWhoseOriginalsDiffer: under "Archive" the +// original is Archive/x.pdf, under the plain scope it is the older +// x-copy.pdf, so each file is a duplicate somewhere. Neither is deleted; +// both are moved aside. +func TestNoDeleteWithTwoScopesWhoseOriginalsDiffer(t *testing.T) { + h, dl := dlTree(t, map[string]int{"Archive/x.pdf": 1, "x-copy.pdf": 0}, sameBytes) + dp := planAndApply(t, h, ` +(rule "pdfs" (when (type pdf)) (delete permanent)) +(rule "archive-dupes" (when (duplicate "Archive")) (move "Dupes") (stop)) +(rule "local-dupes" (when (duplicate)) (move "Dupes") (stop)) +`) + assertCopies(t, dl, sameBytes, []string{"Dupes/x.pdf", "Dupes/x-copy.pdf"}) + assertSkip(t, dp, "Archive/x.pdf", "pdfs", NeverDeleted) + assertSkip(t, dp, "x-copy.pdf", "pdfs", NeverDeleted) +} + +// TestDeleteWithoutDuplicateTestsStillDeletes: the guarantee applies only +// where a directory's rules use (duplicate); a plain delete rule still does +// what it says. +func TestDeleteWithoutDuplicateTestsStillDeletes(t *testing.T) { + h, dl := dlTree(t, map[string]int{"a.pdf": 0, "b.pdf": 1}, sameBytes) + planAndApply(t, h, `(rule "pdfs" (when (type pdf)) (delete permanent))`) + assertCopies(t, dl, sameBytes, nil) +} + +// TestNoDeleteWhenTheDuplicateCheckFails: b.pdf cannot be read, so krino +// cannot show it is not a duplicate; its delete is skipped rather than +// guessed. a.pdf, whose only candidate could not be hashed, is not a +// duplicate and is deleted. +func TestNoDeleteWhenTheDuplicateCheckFails(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads files regardless of their mode") + } + h, dl := dlTree(t, map[string]int{"a.pdf": 0, "b.pdf": 1}, sameBytes) + b := filepath.Join(dl, "b.pdf") + if err := os.Chmod(b, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(b, 0o644) }) + dp := planAndApply(t, h, ` +(rule "dupes" (when (size > 1G) (duplicate)) (move "Dupes")) +(rule "pdfs" (when (type pdf)) (delete)) +`) + assertSkip(t, dp, "b.pdf", "pdfs", "duplicate check failed, so not deleted: ") + if _, err := os.Lstat(b); err != nil { + t.Errorf("b.pdf is gone: %v", err) + } + if _, err := os.Lstat(filepath.Join(dl, "a.pdf")); !os.IsNotExist(err) { + t.Errorf("a.pdf was not deleted: %v", err) + } +} diff --git a/internal/engine/plan.go b/internal/engine/plan.go index 979bec1..d9ffeb3 100644 --- a/internal/engine/plan.go +++ b/internal/engine/plan.go @@ -52,7 +52,7 @@ func (e *Engine) Plan(ctx context.Context, d *Dir, claims *plan.Claims) (*DirPla Reasons: rm.Reasons, } } - inputs[i] = plan.Input{File: fm.File, Rules: rules} + inputs[i] = plan.Input{File: fm.File, Rules: rules, NoDelete: fm.NoDelete} } chains := plan.Build(d.Root, inputs, e.Now(), plan.OS{}, claims) diff --git a/internal/plan/chain.go b/internal/plan/chain.go index bfa2484..e504504 100644 --- a/internal/plan/chain.go +++ b/internal/plan/chain.go @@ -32,6 +32,10 @@ func NewClaims() *Claims { type Input struct { File scan.File Rules []RuleMatch + // NoDelete, when non-empty, skips every delete step of this file with + // this text as the step's Skip, without ending the chain (spec §5.5 + // rule 2, §7.1). + NoDelete string } // Build turns each file's matching rules into a chain. root is the @@ -166,6 +170,10 @@ func buildOne(root string, in Input, now time.Time, d Disk, claim claimed) Chain } case config.Delete, config.DeletePermanent: + if in.NoDelete != "" { + step.Skip = in.NoDelete + break + } deletedBy = rule.Name } diff --git a/internal/plan/chain_test.go b/internal/plan/chain_test.go index b6e3dfe..28c208f 100644 --- a/internal/plan/chain_test.go +++ b/internal/plan/chain_test.go @@ -141,3 +141,31 @@ func TestBuildKeepsSteplessChains(t *testing.T) { } } } + +// TestBuildNoDeleteSkipsDeletesAndContinues is spec §5.5 rule 2 and §7.1: +// with Input.NoDelete set, every delete step is skipped with that reason, +// and the chain carries on from the file's current path instead of ending. +func TestBuildNoDeleteSkipsDeletesAndContinues(t *testing.T) { + in := []Input{{ + File: file("/r", "b.pdf"), + NoDelete: "a duplicate is never deleted", + Rules: []RuleMatch{ + {Name: "old", Actions: []config.Action{act(config.DeletePermanent, "")}}, + {Name: "dupes", Actions: []config.Action{act(config.Move, "Dupes")}}, + {Name: "cleanup", Actions: []config.Action{act(config.Delete, "")}}, + }, + }} + steps := Build("/r", in, time.Now(), NoDisk{}, NewClaims())[0].Steps + if len(steps) != 3 { + t.Fatalf("steps = %+v", steps) + } + if steps[0].Kind != DeletePermanent || steps[0].Skip != "a duplicate is never deleted" { + t.Errorf("step 0 = %+v, want a skipped permanent delete", steps[0]) + } + if steps[1].Kind != Move || steps[1].Skip != "" || steps[1].Dst != "/r/Dupes/b.pdf" { + t.Errorf("step 1 = %+v, want the move to run", steps[1]) + } + if steps[2].Kind != Trash || steps[2].Skip != "a duplicate is never deleted" || steps[2].Src != "/r/Dupes/b.pdf" { + t.Errorf("step 2 = %+v, want a skipped trash from the moved path", steps[2]) + } +} diff --git a/man/krino.conf.5 b/man/krino.conf.5 index fbd1c06..0778779 100644 --- a/man/krino.conf.5 +++ b/man/krino.conf.5 @@ -254,6 +254,20 @@ A rule with only .Ic (stop) is an exclusion: files it matches receive no actions from later rules. .Pp +A rule whose condition contains +.Ic (duplicate) +anywhere, including inside +.Ic or +or +.Ic not , +cannot contain +.Ic (delete) +or +.Ic (delete permanent) ; +.Ic krino check +and every run refuse it +.Pq Sx DUPLICATES . +.Pp .Ar dest is a directory: a relative path is relative to the root, .Ql ~ @@ -462,6 +476,11 @@ its directory, and later steps use the new path; .Ic delete ends the chain, and steps after it are shown in the plan as .Dq skipped: deleted by rule Ar x . +A +.Ic delete +of a duplicate is itself skipped +.Pq Sx DUPLICATES +and does not end the chain. .Pp The plan warns when a chain moves a file more than once; that is usually a missing @@ -609,54 +628,62 @@ between and .Dq this should not exist twice exists only in the user's intent, not in the files. -Pairing -.Ic (duplicate) -with -.Ic (delete) -over a directory that may hold intentional copies +A +.Ic move +rule over a directory that holds intentional copies .Pq a mirror, a staging queue, anything another tool manages -will delete things the user meant to keep. -Keep such a rule on a directory scoped narrowly enough that every -byte-identical pair in it really is an accident. +will move things the user meant to keep there; keep such a rule on a +directory scoped narrowly enough that every byte-identical pair in it really +is an accident. .Pp -.Sy Warning: -each +.Sy Duplicates are found, never deleted. +Deciding which copy to remove is left to the user, or to a tool built for it +such as +.Xr jdupes 1 . +A rule combining .Ic (duplicate) -or -.Ic (duplicate Ar dir ) -condition elects its own original from its own candidates; two conditions -with different scopes can therefore elect -.Em different -originals for the same content. -Within one directory's rules -.Pq one rule combining conditions with Ic or , or two separate rules , -this can make every copy in a group selected by some condition, and with -.Ic (delete permanent) -every copy selected is gone for good. -.Ic krino -plans and applies one directory at a time, so a directory already applied -in the run stays applied while a later directory is planned; a -.Fl n -run only ever plans, so it shows every directory's plan as if none of the -others had been applied. -Two files in different directories that a -.Fl n -plan each lists as a duplicate of the other are therefore not necessarily -both deleted when the same run is applied with -.Fl y : -deleting the first can remove the very original the second file was a -duplicate of, leaving the second no longer a duplicate by the time its own -directory is planned. -Use one duplicate scope for rules that delete within a directory; prefer -.Ic (delete) -to -.Ic (delete permanent) -with -.Ic (duplicate) ; -and within one directory's rules, two files each listed as a duplicate of -the other in the -.Fl n -plan means both would be deleted. +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 +\(em each distinct set of +.Ar dir +arguments, and the plain +.Ic (duplicate) , +looked up for the file whether or not evaluation reached that test \(em gets +no delete step from any rule. +The plan shows the step as +.Dq skipped: a duplicate is never deleted , +or, when that lookup fails, +.Dq skipped: duplicate check failed, so not deleted: Ar reason ; +the rest of the chain continues from the file's current path. +This covers what the refusal cannot see: a later rule deleting through +.Ic (matched) +or through a condition of its own. +.Pp +Together they mean no rule can delete every copy of content a duplicate test +in that directory can see. +A file displaced by +.Ic (on-conflict overwrite) +is not covered; it goes to the trash, and +.Ic krino undo +restores it. +.Pp +The way to deal with duplicates is to move them aside and decide later: +.Bd -literal -offset indent +(rule "dupes" + (when (duplicate "~/docs/Archive")) + (move "~/.dupes/") + (stop)) +.Ed +.Pp +Every move is logged, so +.Ic krino undo +puts them back. +Duplicate conditions with different scopes do not share an original, so two +such rules can between them move every copy of a group aside; nothing is +deleted. .Sh KNOWN LIMITATIONS A .Ar dest @@ -682,12 +709,6 @@ Give such a rule its own narrow pattern, or a .Ar dest whose first path component is a literal string, when that matters. -.Pp -.Ic (duplicate) -conditions with different scopes do not share an original: two conditions -electing from different candidate sets can each treat a different file as -the original of the same content -.Pq Sx DUPLICATES . .Sh SEE ALSO .Xr krino 1 .Pp |
