plan 9: apply logs each step as it completes and stops a chain that landed elsewhere

2 files changed, 131 insertions, 19 deletions

diff --git a/internal/apply/apply.go b/internal/apply/apply.go
index 713913e..5624b27 100644
--- a/internal/apply/apply.go
+++ b/internal/apply/apply.go
@@ -42,38 +42,59 @@ type StepResult struct {
 
 // Chain runs one file's steps in order and stops at the first failure,
 // marking the rest skipped. It never touches a file whose size or mtime no
-// longer matches what the plan recorded.
+// longer matches what the plan recorded. It is ChainLogged with no done.
 func Chain(c plan.Chain) []StepResult {
+	results, _ := ChainLogged(c, nil)
+	return results
+}
+
+// ChainLogged is Chain, calling done with each step's result as soon as
+// that step has run or been skipped, before the next one starts - so a
+// caller that logs from done never has a completed step missing from the
+// log when the process dies mid-chain (review M9). An error from done stops
+// the chain at once and is returned with the results so far.
+//
+// A move or rename that had to take a free name at apply time, because its
+// planned destination was taken since planning, stops the chain too: every
+// later step was planned against the name the file did not get, and must
+// not act on whatever is at that path (review M3).
+func ChainLogged(c plan.Chain, done func(i int, sr StepResult) error) ([]StepResult, error) {
 	results := make([]StepResult, len(c.Steps))
-	stopped := false
+	stopWhy := ""
 
 	for i, step := range c.Steps {
-		if stopped {
-			results[i] = StepResult{Step: step, Status: "skipped", Detail: "an earlier step in this chain failed"}
-			continue
-		}
-		if step.Skip != "" {
+		switch {
+		case stopWhy != "":
+			results[i] = StepResult{Step: step, Status: "skipped", Detail: stopWhy}
+		case step.Skip != "":
 			// Planning already decided this step will not run; it must not
 			// be attempted, so no pre-step check, no directory creation, no
 			// touching the file (spec: a step already marked Skip is
 			// reported, not attempted).
 			results[i] = StepResult{Step: step, Status: "skipped", Detail: step.Skip}
-			continue
+		default:
+			if err := checkUnchanged(step.Src, c.File); err != nil {
+				results[i] = StepResult{Step: step, Status: "failed", Detail: err.Error()}
+				stopWhy = "an earlier step in this chain failed"
+				break
+			}
+			res := runStep(step)
+			results[i] = res
+			switch {
+			case res.Status == "failed":
+				stopWhy = "an earlier step in this chain failed"
+			case (step.Kind == plan.Move || step.Kind == plan.Rename) && res.Dst != step.Dst:
+				stopWhy = fmt.Sprintf("an earlier step put the file at %s, not the planned %s", res.Dst, step.Dst)
+			}
 		}
-		if err := checkUnchanged(step.Src, c.File); err != nil {
-			results[i] = StepResult{Step: step, Status: "failed", Detail: err.Error()}
-			stopped = true
-			continue
-		}
-
-		res := runStep(step)
-		results[i] = res
-		if res.Status == "failed" {
-			stopped = true
+		if done != nil {
+			if err := done(i, results[i]); err != nil {
+				return results[:i+1], err
+			}
 		}
 	}
 
-	return results
+	return results, nil
 }
 
 // checkUnchanged is the guard that matters most: before every step, the
@@ -131,6 +152,11 @@ func runFileStep(step plan.Step) StepResult {
 	var displacedEntry string
 
 	if step.Displaces != "" {
+		// Re-checked at apply time (review M4): only a regular file may be
+		// trashed to make room, never a directory or link put there since.
+		if fi, err := os.Lstat(step.Displaces); err != nil || !fi.Mode().IsRegular() {
+			return StepResult{Step: step, Status: "failed", Detail: "the file to replace is gone or no longer a regular file"}
+		}
 		// overwrite policy: the file already at dst must be trashed before
 		// this step's own destination name is used, so no free-name search
 		// applies here — the whole point of displacing was to clear this
diff --git a/internal/apply/swap_test.go b/internal/apply/swap_test.go
index fe7542e..c185686 100644
--- a/internal/apply/swap_test.go
+++ b/internal/apply/swap_test.go
@@ -99,3 +99,89 @@ func TestChainFollowsItsOwnFile(t *testing.T) {
 		}
 	}
 }
+
+// TestChainStopsWhenAStepLandsElsewhere: a move that found its planned name
+// taken at apply time lands at a free name, and every later step - planned
+// against the name it did not get - is skipped rather than acting on
+// whatever is at the planned path (review M3).
+func TestChainStopsWhenAStepLandsElsewhere(t *testing.T) {
+	root := t.TempDir()
+	p := filepath.Join(root, "a.pdf")
+	plannedPath := filepath.Join(root, "W", "a.pdf")
+	c := planned(t, root, "a.pdf", "planned-file",
+		plan.Step{Kind: plan.Move, Src: p, Dst: plannedPath},
+		plan.Step{Kind: plan.DeletePermanent, Src: plannedPath},
+	)
+	if err := os.MkdirAll(filepath.Dir(plannedPath), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(plannedPath, []byte("OTHER--FILE!"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chtimes(plannedPath, c.File.ModTime, c.File.ModTime); err != nil {
+		t.Fatal(err)
+	}
+	res := Chain(c)
+	if res[0].Status != "ok" || res[1].Status != "skipped" || !strings.Contains(res[1].Detail, "not the planned") {
+		t.Fatalf("steps = %s %q, %s %q; want ok, then skipped naming the planned path", res[0].Status, res[0].Dst, res[1].Status, res[1].Detail)
+	}
+	if b, err := os.ReadFile(plannedPath); err != nil || string(b) != "OTHER--FILE!" {
+		t.Errorf("the file at the planned path was acted on: %q %v", b, err)
+	}
+}
+
+// TestChainLoggedReportsEachStepBeforeTheNext: done is called for each step
+// as soon as it has run - the file is at the first step's destination and
+// the second step has not happened yet - so a caller logging from done
+// never has a completed step missing from the log (review M9).
+func TestChainLoggedReportsEachStepBeforeTheNext(t *testing.T) {
+	root := t.TempDir()
+	p := filepath.Join(root, "a.pdf")
+	moved := filepath.Join(root, "W", "a.pdf")
+	renamed := filepath.Join(root, "W", "b.pdf")
+	c := planned(t, root, "a.pdf", "body",
+		plan.Step{Kind: plan.Move, Src: p, Dst: moved},
+		plan.Step{Kind: plan.Rename, Src: moved, Dst: renamed},
+	)
+	var calls []int
+	res, err := ChainLogged(c, func(i int, sr StepResult) error {
+		calls = append(calls, i)
+		if i == 0 {
+			if _, err := os.Lstat(moved); err != nil {
+				t.Errorf("at done(0) the move has not happened: %v", err)
+			}
+			if _, err := os.Lstat(renamed); !os.IsNotExist(err) {
+				t.Errorf("at done(0) the rename already happened")
+			}
+		}
+		return nil
+	})
+	if err != nil || len(calls) != 2 || res[1].Status != "ok" {
+		t.Fatalf("calls %v, err %v, results %+v", calls, err, res)
+	}
+}
+
+// TestChainRefusesToDisplaceANonRegularTarget: the file overwrite was
+// planned to replace is re-checked before it is trashed; a directory now in
+// its place is left alone (review M4).
+func TestChainRefusesToDisplaceANonRegularTarget(t *testing.T) {
+	root := t.TempDir()
+	t.Setenv("HOME", root)
+	t.Setenv("XDG_DATA_HOME", filepath.Join(root, "share"))
+	t.Setenv("XDG_STATE_HOME", "")
+	t.Setenv("XDG_CONFIG_HOME", "")
+	t.Setenv("XDG_CACHE_HOME", "")
+	p := filepath.Join(root, "a.pdf")
+	target := filepath.Join(root, "W", "a.pdf")
+	c := planned(t, root, "a.pdf", "body", plan.Step{Kind: plan.Move, Src: p, Dst: target, Displaces: target})
+	if err := os.MkdirAll(filepath.Join(target, "inside"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	res := Chain(c)
+	if res[0].Status != "failed" {
+		t.Fatalf("step = %s %q; want failed", res[0].Status, res[0].Detail)
+	}
+	if _, err := os.Stat(filepath.Join(target, "inside")); err != nil {
+		t.Errorf("the directory was displaced: %v", err)
+	}
+}