From bcfeb773afcc28641ef2fb47749c91fd0767054a Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 16 Sep 2026 09:17:08 +0200 Subject: gui: the Plan tab - scan, review, apply, with the directory locked --- gui/internal/model/plan.go | 37 +++++++++++-- gui/internal/model/plan_test.go | 118 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 4 deletions(-) (limited to 'gui/internal/model') diff --git a/gui/internal/model/plan.go b/gui/internal/model/plan.go index 607fa29..7496347 100644 --- a/gui/internal/model/plan.go +++ b/gui/internal/model/plan.go @@ -11,6 +11,7 @@ import ( "sort" "krino/internal/engine" + "krino/internal/lock" "krino/internal/plan" ) @@ -40,6 +41,7 @@ type PlanTab struct { sess *engine.Session dp *engine.DirPlan + lock *lock.Lock } // Counts is the plan's summary line. @@ -47,19 +49,42 @@ type Counts struct { Scanned, Acting, Excluded, Skipped, Unmatched, Warned int } -// Plan locks dir and plans it, returning the tab to show. The lock is held -// until Apply or Close: what the user sees stays the truth while they -// choose (GUI design §3). +// Plan locks d and plans it, returning the tab to show. The lock is held +// until Close, so what the window shows stays the truth while the user +// chooses, and no other krino moves the files under them (GUI design §3). +// A directory another run is already working in is not planned at all: the +// error is lock.ErrHeld, naming the directory. func Plan(ctx context.Context, sess *engine.Session, d *engine.Dir) (*PlanTab, error) { + l, err := sess.Lock(ctx, d, false) + if err != nil { + return nil, fmt.Errorf("%s: %w", d.Name, err) + } + // Start from what this run has actually landed: a name a plan the user + // closed had reserved is free again, so looking twice never creeps up + // through name-1, name-2 (spec §7.4). + sess.FinishDirectory() dp, err := sess.Plan(ctx, d) if err != nil { + l.Release() return nil, err } - t := &PlanTab{Dir: d, sess: sess, dp: dp} + t := &PlanTab{Dir: d, sess: sess, dp: dp, lock: l} t.fill() return t, nil } +// Close releases the directory's lock. The session outlives the tab, so +// closing one plan to open another keeps the run - and its claims - going. +// Closing twice is not an error. +func (t *PlanTab) Close() error { + if t.lock == nil { + return nil + } + l := t.lock + t.lock = nil + return l.Release() +} + // fill turns the engine's plan into rows and counts. func (t *PlanTab) fill() { r := t.dp.Result @@ -192,6 +217,10 @@ func (t *PlanTab) Apply(ctx context.Context) (*engine.ApplyResult, error) { } res, err := t.sess.Apply(ctx, t.dp, approved) t.Applied = true + // The disk is now the truth: a destination this plan reserved but never + // used is free again, while one it did use stays protected for the rest + // of the run (spec §7.4). + t.sess.FinishDirectory() if res != nil { t.record(res) } diff --git a/gui/internal/model/plan_test.go b/gui/internal/model/plan_test.go index c157b1d..8b7e32f 100644 --- a/gui/internal/model/plan_test.go +++ b/gui/internal/model/plan_test.go @@ -4,13 +4,16 @@ package model import ( "context" + "errors" "os" "path/filepath" + "sort" "strings" "testing" "time" "krino/internal/engine" + "krino/internal/lock" "krino/internal/plan" ) @@ -185,3 +188,118 @@ func TestPlanLeavesTheDirectoryAlone(t *testing.T) { t.Errorf("planning created the destination: %v", err) } } + +// TestPlanHoldsTheLock: while a plan is open nothing else may work in that +// directory - what the window shows stays the truth while the user chooses - +// and closing the tab lets the next run in (GUI design §3). +func TestPlanHoldsTheLock(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n" + e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one"}) + tab, _ := planTab(t, e) + if _, err := lock.Acquire(context.Background(), e.Config.LockFile("dl"), false); !errors.Is(err, lock.ErrHeld) { + t.Fatalf("an open plan does not hold the lock: %v", err) + } + if err := tab.Close(); err != nil { + t.Fatal(err) + } + l, err := lock.Acquire(context.Background(), e.Config.LockFile("dl"), false) + if err != nil { + t.Fatalf("closing the tab did not release the lock: %v", err) + } + l.Release() + if err := tab.Close(); err != nil { + t.Errorf("closing twice: %v", err) + } +} + +// TestPlanRefusesAHeldDirectory: a directory another krino is working in is +// not planned at all, and the message names it. +func TestPlanRefusesAHeldDirectory(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n" + e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one"}) + held, err := lock.Acquire(context.Background(), e.Config.LockFile("dl"), false) + if err != nil { + t.Fatal(err) + } + defer held.Release() + s, err := e.NewSession(false) + if err != nil { + t.Fatal(err) + } + defer s.Close() + if _, err := Plan(context.Background(), s, e.Dirs[0]); !errors.Is(err, lock.ErrHeld) { + t.Fatalf("Plan = %v, want lock.ErrHeld", err) + } else if !strings.Contains(err.Error(), "dl") { + t.Errorf("the message does not name the directory: %v", err) + } +} + +// TestRescanForgetsUnusedNames: a destination the last plan claimed but +// never used is free again on the next scan, so looking twice at the same +// directory does not creep up through name-1, name-2 (spec §7.4). +func TestRescanForgetsUnusedNames(t *testing.T) { + conf := "(path \"~/dl\")\n(rule \"all\" (move \"Out\") (rename \"same.pdf\"))\n" + e, _ := sandboxDir(t, conf, map[string]string{"a.pdf": "one", "b.pdf": "two"}) + tab, s := planTab(t, e) + // Each file moves into Out and is then renamed; the second file's name + // is taken, so the plan reserves a suffixed one for it. + want := destinations(t, tab) + if len(want) != 4 || filepath.Base(want[2]) != "same.pdf" || filepath.Base(want[3]) != "same_1.pdf" { + t.Fatalf("first plan = %v", want) + } + // Looking again without applying anything: the names the closed plan + // reserved are free, so the second look reads the same as the first. + if err := tab.Close(); err != nil { + t.Fatal(err) + } + again, err := Plan(context.Background(), s, e.Dirs[0]) + if err != nil { + t.Fatal(err) + } + if got := destinations(t, again); !equal(got, want) { + t.Errorf("second plan = %v, want %v", got, want) + } + // And again after an apply that applied nothing. + again.SelectNone() + if _, err := again.Apply(context.Background()); err != nil { + t.Fatal(err) + } + if err := again.Close(); err != nil { + t.Fatal(err) + } + third, err := Plan(context.Background(), s, e.Dirs[0]) + if err != nil { + t.Fatal(err) + } + defer third.Close() + if got := destinations(t, third); !equal(got, want) { + t.Errorf("third plan = %v, want %v", got, want) + } +} + +// destinations is every step's destination in the tab, sorted. +func destinations(t *testing.T, tab *PlanTab) []string { + t.Helper() + var out []string + for _, r := range tab.Rows { + for _, st := range r.Steps { + if st.Dst != "" { + out = append(out, st.Dst) + } + } + } + sort.Strings(out) + return out +} + +func equal(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} -- cgit v1.3