undo runs through the same session

4 files changed, 114 insertions, 51 deletions

diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index 4c0a0d8..54d5574 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -104,6 +104,13 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 		fmt.Fprintf(stderr, "krino: %v\n", err)
 		return 1
 	}
+	// Sorting opens the log before the first directory, as it always has:
+	// the run id is fixed for every directory, and a failure to open is
+	// reported before anything is planned.
+	if err := sess.OpenLog(); err != nil {
+		fmt.Fprintf(stderr, "krino: %v\n", err)
+		return 1
+	}
 	defer func() {
 		if cerr := sess.Close(); cerr != nil {
 			fmt.Fprintf(stderr, "krino: %v\n", cerr)
diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go
index ecab2f3..5a7db4b 100644
--- a/cmd/krino/undo.go
+++ b/cmd/krino/undo.go
@@ -15,9 +15,7 @@ import (
 
 	"golang.org/x/term"
 
-	"krino/internal/config"
 	"krino/internal/engine"
-	"krino/internal/journal"
 	"krino/internal/lock"
 	"krino/internal/xdg"
 )
@@ -113,7 +111,18 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int {
 
 	// PlanUndo only reads the log; nothing is touched yet (spec §10), which
 	// is what makes it safe to call before any lock is taken.
-	up, err := e.PlanUndo(runID)
+	sess, err := e.NewSession(g.dry)
+	if err != nil {
+		fmt.Fprintf(stderr, "krino: %v\n", err)
+		return 1
+	}
+	defer func() {
+		if cerr := sess.Close(); cerr != nil {
+			fmt.Fprintf(stderr, "krino: %v\n", cerr)
+		}
+	}()
+
+	up, err := sess.PlanUndo(runID)
 	if err != nil {
 		fmt.Fprintf(stderr, "krino: %v\n", err)
 		return 1
@@ -137,7 +146,7 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int {
 	// right after acquisition.
 	var locks []*lock.Lock
 	if !g.dry {
-		locks, err = acquireUndoLocks(ctx, e.Config, undoDirNames(up.Files), !g.yes)
+		locks, err = sess.LockDirs(ctx, undoDirNames(up.Files), !g.yes)
 		if err != nil {
 			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
 				return 130
@@ -197,26 +206,12 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int {
 
 	// Ruling 2 (Task 7), carried over: journal.Open creates the state
 	// directory and the log file as a side effect of merely being called,
-	// so it is opened only once we know something will actually be
+	// so the session opens it only now, when something will actually be
 	// applied - never for -n (returned above), and not merely because -y
-	// or a review session ran, unlike cmdSort's own eager-open (which opens
-	// before it knows whether anything is actionable, a difference forced
-	// by cmdSort not yet having a plan to inspect at that point in its
-	// flow; undo already does, so it opens later, and never opens if
-	// undoActionableCount was 0 or the user chose [s]/[q] above).
-	j, err := journal.Open(e.Config.LogFile())
-	if err != nil {
-		fmt.Fprintf(stderr, "krino: %v\n", err)
-		return 1
-	}
-	defer func() {
-		if cerr := j.Close(); cerr != nil {
-			fmt.Fprintf(stderr, "krino: %v\n", cerr)
-		}
-	}()
-	run := journal.NewRunID(e.Now())
-
-	res, aerr := e.ApplyUndo(ctx, toApply, j, run)
+	// or a review session ran, unlike cmdSort, which opens before it knows
+	// whether anything is actionable (a difference forced by cmdSort not
+	// yet having a plan to inspect at that point in its flow).
+	res, aerr := sess.ApplyUndo(ctx, toApply)
 	if aerr != nil {
 		if errors.Is(aerr, context.Canceled) || errors.Is(aerr, context.DeadlineExceeded) {
 			// Interrupted mid-apply: the ctx.Err() check below turns this
@@ -269,24 +264,6 @@ func undoDirNames(files []engine.UndoFile) []string {
 	return out
 }
 
-// acquireUndoLocks takes the lock for every name in dirs, in order,
-// mirroring cmdSort's per-directory lock.Acquire call. If any acquisition
-// fails - held with wait false, or ctx cancelled while waiting - every lock
-// already taken is released before returning, so a partial lock set is
-// never left held while the caller reports the error and stops.
-func acquireUndoLocks(ctx context.Context, cfg *config.Config, dirs []string, wait bool) ([]*lock.Lock, error) {
-	locks := make([]*lock.Lock, 0, len(dirs))
-	for _, name := range dirs {
-		l, err := lock.Acquire(ctx, cfg.LockFile(name), wait)
-		if err != nil {
-			releaseUndoLocks(locks)
-			return nil, fmt.Errorf("%s: %w", name, err)
-		}
-		locks = append(locks, l)
-	}
-	return locks, nil
-}
-
 // releaseUndoLocks releases every lock in locks and returns any release
 // errors, one lock's failure never stopping the rest from being released -
 // the same "release on every path" guarantee cmdSort gives its own single
diff --git a/internal/engine/session.go b/internal/engine/session.go
index 8d52027..33b6a76 100644
--- a/internal/engine/session.go
+++ b/internal/engine/session.go
@@ -28,22 +28,32 @@ type Session struct {
 	dry    bool
 }
 
-// NewSession starts a run. A real one opens the log; the caller closes the
-// session when the run is over.
+// NewSession starts a run. The log is opened by OpenLog, or by the first
+// Apply or ApplyUndo: sorting opens it before it plans anything, while undo
+// opens it only once it knows it will reverse something, and neither must
+// create a log a dry run would not. The caller closes the session when the
+// run is over.
 func (e *Engine) NewSession(dry bool) (*Session, error) {
-	s := &Session{e: e, claims: plan.NewClaims(), dry: dry}
-	if dry {
-		return s, nil
+	return &Session{e: e, claims: plan.NewClaims(), dry: dry}, nil
+}
+
+// OpenLog opens the log and takes the run id, once. A dry session does
+// neither: journal.Open creates the state directory and an empty krino.log
+// merely by being called (spec §11).
+func (s *Session) OpenLog() error {
+	if s.dry || s.j != nil {
+		return nil
 	}
-	j, err := journal.Open(e.Config.LogFile())
+	j, err := journal.Open(s.e.Config.LogFile())
 	if err != nil {
-		return nil, err
+		return err
 	}
-	s.j, s.run = j, journal.NewRunID(e.Now())
-	return s, nil
+	s.j, s.run = j, journal.NewRunID(s.e.Now())
+	return nil
 }
 
-// Run is the run id every entry of this session carries; "" for a dry one.
+// Run is the run id every entry of this session carries; "" for a dry one,
+// and until the log is open.
 func (s *Session) Run() string { return s.run }
 
 // Journal is the log this session writes, nil for a dry one.
@@ -87,6 +97,9 @@ func (s *Session) Plan(ctx context.Context, d *Dir) (*DirPlan, error) {
 // overwrite) from displacing this run's own result (spec §7.4). A dry
 // session keeps every claim, since it applies nothing.
 func (s *Session) Apply(ctx context.Context, dp *DirPlan, approved map[string]bool) (*ApplyResult, error) {
+	if err := s.OpenLog(); err != nil {
+		return nil, err
+	}
 	res, err := s.e.Apply(ctx, dp, approved, s.j, s.run)
 	if !s.dry {
 		s.claims = plan.NewClaims()
@@ -133,6 +146,9 @@ func (s *Session) PlanUndo(runID string) (*UndoPlan, error) {
 
 // ApplyUndo carries out up and logs it under this session's run id.
 func (s *Session) ApplyUndo(ctx context.Context, up *UndoPlan) (*ApplyResult, error) {
+	if err := s.OpenLog(); err != nil {
+		return nil, err
+	}
 	return s.e.ApplyUndo(ctx, up, s.j, s.run)
 }
 
diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go
index ece69dd..8fd4593 100644
--- a/internal/engine/session_test.go
+++ b/internal/engine/session_test.go
@@ -47,6 +47,9 @@ func TestSessionKeepsOnlyAppliedDestinationsClaimed(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer s.Close()
+	if err := s.OpenLog(); err != nil {
+		t.Fatal(err)
+	}
 	if s.Run() == "" {
 		t.Error("a real session has no run id")
 	}
@@ -79,6 +82,9 @@ func TestDrySessionWritesNoLog(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer s.Close()
+	if err := s.OpenLog(); err != nil {
+		t.Fatal(err)
+	}
 	if s.Run() != "" {
 		t.Errorf("a dry session took a run id: %q", s.Run())
 	}
@@ -117,3 +123,60 @@ func TestSessionLockIsTheDirectorysOwn(t *testing.T) {
 		l2.Release()
 	}
 }
+
+// TestSessionUndoReversesTheRun: undo goes through the same session as
+// sorting - its locks, its log, its run id (GUI design §1.3).
+func TestSessionUndoReversesTheRun(t *testing.T) {
+	e, run, h, _ := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}},
+		map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"})
+	s, err := e.NewSession(false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	locks, err := s.LockDirs(context.Background(), []string{"dl"}, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	up, err := s.PlanUndo(run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.ApplyUndo(context.Background(), up); err != nil {
+		t.Fatal(err)
+	}
+	for _, l := range locks {
+		l.Release()
+	}
+	if _, err := os.Stat(filepath.Join(h, "dl", "a.pdf")); err != nil {
+		t.Errorf("undo did not put the file back: %v", err)
+	}
+}
+
+// TestSessionLockDirsReleasesOnFailure: when one directory's lock cannot be
+// had, the locks already taken are released, so a failed undo leaves none
+// held.
+func TestSessionLockDirsReleasesOnFailure(t *testing.T) {
+	h := sandbox(t)
+	e := twoOverwritingDirs(t, h)
+	s, err := e.NewSession(true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	held, err := s.Lock(context.Background(), e.Dirs[1], false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.LockDirs(context.Background(), []string{"a", "b"}, false); err == nil {
+		t.Fatal("LockDirs took a lock another holder had")
+	}
+	held.Release()
+	locks, err := s.LockDirs(context.Background(), []string{"a", "b"}, false)
+	if err != nil {
+		t.Fatalf("the first directory's lock was left held: %v", err)
+	}
+	for _, l := range locks {
+		l.Release()
+	}
+}