3 files changed, 54 insertions, 1 deletions
diff --git a/internal/plan/chain.go b/internal/plan/chain.go
index d4d07da..793e0e1 100644
--- a/internal/plan/chain.go
+++ b/internal/plan/chain.go
@@ -68,6 +68,14 @@ func Build(root string, in []Input, now time.Time, d Disk, claims *Claims) []Cha
 		return in[order[i]].File.Rel < in[order[j]].File.Rel
 	})
 
+	// Every scanned file's own path counts as spoken for (review M4): no step
+	// of this plan may trash another scanned file to make room - it may be
+	// about to move away, or be acted on by its own chain - so overwrite
+	// takes a free name there, exactly as suffix already did for a path that
+	// exists on disk.
+	for _, x := range in {
+		claims.taken[x.File.Path] = true
+	}
 	chains := make([]Chain, len(in))
 	for _, i := range order {
 		chains[i] = buildOne(root, in[i], now, d, claims.taken)
diff --git a/internal/plan/conflict.go b/internal/plan/conflict.go
index 8747669..c4c6b64 100644
--- a/internal/plan/conflict.go
+++ b/internal/plan/conflict.go
@@ -15,6 +15,7 @@ import (
 // tests can supply a stub and Build stays pure otherwise.
 type Disk interface {
 	Exists(path string) bool
+	Regular(path string) bool
 	SameContent(a, b string) (bool, error)
 }
 
@@ -29,6 +30,13 @@ func (OS) Exists(path string) bool {
 	return err == nil
 }
 
+// Regular reports whether path is a regular file, not following a symlink
+// at path itself.
+func (OS) Regular(path string) bool {
+	fi, err := os.Lstat(path)
+	return err == nil && fi.Mode().IsRegular()
+}
+
 // SameContent delegates to internal/dup, the one place content identity is
 // decided.
 func (OS) SameContent(a, b string) (bool, error) {
@@ -40,6 +48,7 @@ func (OS) SameContent(a, b string) (bool, error) {
 type NoDisk struct{}
 
 func (NoDisk) Exists(string) bool                       { return false }
+func (NoDisk) Regular(string) bool                      { return false }
 func (NoDisk) SameContent(string, string) (bool, error) { return false, nil }
 
 // claimed is the set of destination paths already spoken for by an earlier
@@ -89,6 +98,12 @@ func resolveConflict(kind Kind, policy config.Conflict, src, dst string, d Disk,
 		return dst, "target exists", ""
 	case config.ConflictOverwrite:
 		if onDisk && !c[dst] {
+			// Only a regular file is ever trashed to make room (review M4):
+			// a directory or link of the same name stays, and so does the
+			// step - skipped, saying why.
+			if !d.Regular(dst) {
+				return dst, "target is not a regular file", ""
+			}
 			// The existing file is trashed first (plan 4). Only the first
 			// step to reach this path may displace it: once another step
 			// in this same plan has already claimed dst, that path will
diff --git a/internal/plan/conflict_test.go b/internal/plan/conflict_test.go
index bdc37af..289cd14 100644
--- a/internal/plan/conflict_test.go
+++ b/internal/plan/conflict_test.go
@@ -16,9 +16,11 @@ import (
 type fakeDisk struct {
 	exists map[string]bool
 	same   map[[2]string]bool
+	dirs   map[string]bool // existing paths that are directories
 }
 
-func (f fakeDisk) Exists(p string) bool { return f.exists[p] }
+func (f fakeDisk) Exists(p string) bool  { return f.exists[p] }
+func (f fakeDisk) Regular(p string) bool { return f.exists[p] && !f.dirs[p] }
 func (f fakeDisk) SameContent(a, b string) (bool, error) {
 	return f.same[[2]string{a, b}], nil
 }
@@ -208,6 +210,7 @@ func TestSameContentReal(t *testing.T) {
 type stubAlwaysExists struct{}
 
 func (stubAlwaysExists) Exists(string) bool                       { return true }
+func (stubAlwaysExists) Regular(string) bool                      { return true }
 func (stubAlwaysExists) SameContent(string, string) (bool, error) { return false, nil }
 
 // TestSuffixedCapsAttempts is C2: suffixed() gives up after
@@ -222,3 +225,30 @@ func TestSuffixedCapsAttempts(t *testing.T) {
 		t.Errorf("step = %+v; want Skip \"too many conflicting names\" and empty Dst", s)
 	}
 }
+
+// TestOverwriteNeverDisplacesADirectory: a hostile file named like an
+// existing directory must not have that directory trashed in its place
+// (review M4).
+func TestOverwriteNeverDisplacesADirectory(t *testing.T) {
+	dir := "/home/x/Documents/Invoices"
+	d := fakeDisk{exists: map[string]bool{dir: true}, dirs: map[string]bool{dir: true}}
+	_, skip, displaces := resolveConflict(Move, config.ConflictOverwrite, "/r/Invoices", dir, d, claimed{})
+	if displaces != "" || skip != "target is not a regular file" {
+		t.Errorf("skip %q displaces %q; want skipped, nothing displaced", skip, displaces)
+	}
+}
+
+// TestOverwriteNeverDisplacesAnotherScannedFile: a file of the same plan is
+// not trashed to make room - it may be about to move away or be acted on -
+// so the step takes a free name instead (review M4).
+func TestOverwriteNeverDisplacesAnotherScannedFile(t *testing.T) {
+	d := fakeDisk{exists: map[string]bool{"/r/a.pdf": true, "/r/b.pdf": true}}
+	in := []Input{
+		{File: file("/r", "a.pdf"), Rules: []RuleMatch{{Name: "r", Settings: config.Resolved{OnConflict: config.ConflictOverwrite}, Actions: []config.Action{act(config.Rename, "b.pdf")}}}},
+		{File: file("/r", "b.pdf"), Rules: []RuleMatch{{Name: "m", Actions: []config.Action{act(config.Move, "Out")}}}},
+	}
+	s := Build("/r", in, time.Now(), d, NewClaims())[0].Steps[0]
+	if s.Displaces != "" || s.Dst != "/r/b_1.pdf" || s.Skip != "" {
+		t.Errorf("Displaces %q Dst %q Skip %q; want no displacement and /r/b_1.pdf", s.Displaces, s.Dst, s.Skip)
+	}
+}