diff --git a/CHANGELOG.md b/CHANGELOG.md index d2e2b389..c2f5d9b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,21 @@ example a hook-less desktop chat sharing a connector. Its calls can then be credited to the session that did declare, so the report remains a proxy. +- **The file watcher leaves linked git worktrees out.** The OS watcher a + workspace's topology index and language servers share now drops every event + inside a linked worktree of the workspace's repository (or of one of its + submodules), never expands one that arrives whole, and stays quiet while one + is removed, since git's own records outlive the checkout. Agents' worktrees + inside a workspace made each checkout or removal a burst of thousands of + events that overflowed the watcher and forced resyncs: one workspace logged + 583k lost-event warnings in a day. The index's own walk uses the same rule, + which now verifies a worktree through git's `commondir` and back-link rather + than the shape of its path, so a submodule at a path like `libs/worktrees/x` + is no longer mistaken for one and a hand-written `.git` pointer proves + nothing. A workspace that is itself a linked worktree now leaves out the + sibling worktrees nested inside it too. `[topology] index_worktrees` keeps + them, on a watcher of its own. + - **The Claude Code and Codex hooks stay inside their session's namespace.** `plumb hooks run-claude` and `run-codex` now send the session's `PLUMB_NAMESPACE` with every daemon request, and the daemon answers the diff --git a/docs/configuration.md b/docs/configuration.md index e8cc3626..8426c046 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -736,6 +736,7 @@ language and resolved path, or the reason it will not run. | `resync_batch` | int | `100` | Files the full resync extracts before pausing, to throttle CPU. `0` disables pacing. | | `resync_pause_ms` | int | `25` | Pause (milliseconds) after each `resync_batch` files. `0` disables pacing. | | `resync_interval_minutes` | int | `60` | Periodic full-resync **fallback**, used only when `watch = false` or the platform watcher cannot start; suppressed while the watcher is live. `0` disables. | +| `index_worktrees` | bool | `false` | Also index, and watch, the workspace's linked git worktrees (`git worktree add`, of its repository or of one of its submodules). Off by default: each is a second full copy of the tree, and agents create and remove them by the dozen. Only a worktree git's own bookkeeping verifies (its `commondir` and back-link) is left out; submodules always stay. With it off, the OS watcher the index shares with the language servers drops every event inside such a worktree; with it on, the index uses a watcher of its own. | | `watch` | bool | `true` | OS-level file watching (`internal/fswatch`: FSEvents on macOS, inotify on Linux; other platforms fall back to `resync_interval_minutes`): re-index a file the instant it changes on disk, whoever changed it — this agent, another agent, or your editor. Replaces time-based polling; a mass change (e.g. `git checkout`) coalesces to a single paced resync via the bounded queue + overflow path. Set `false` to fall back to `resync_interval_minutes`. | Note the two different meanings of `0` in this table: `max_file_size_bytes = 0` means *use the default*, and `extract_timeout_seconds = 0` means *use the built-in 2-minute ceiling* -- it no longer disables bounding entirely, since an unbounded parse can wedge the indexer. diff --git a/internal/cli/pool_descope_test.go b/internal/cli/pool_descope_test.go index 4b890b2c..9e42fd26 100644 --- a/internal/cli/pool_descope_test.go +++ b/internal/cli/pool_descope_test.go @@ -22,12 +22,25 @@ import ( // lockProbe records whether a mutex was free when probed. type lockProbe struct{ heldOnce atomic.Bool } +// lockProbeWindow is how long mu must stay unavailable for the probe to say +// the caller holds it. One failed TryLock is not evidence: the test's own +// concurrent acquires take mu for a moment, and catching one of them flaked +// CI. A caller that holds mu across the probed call holds it throughout. +const lockProbeWindow = 200 * time.Millisecond + func (lp *lockProbe) probe(mu *sync.Mutex) { - if mu.TryLock() { - mu.Unlock() - return + deadline := time.Now().Add(lockProbeWindow) + for { + if mu.TryLock() { + mu.Unlock() + return + } + if time.Now().After(deadline) { + lp.heldOnce.Store(true) + return + } + time.Sleep(time.Millisecond) } - lp.heldOnce.Store(true) } func within(t *testing.T, d time.Duration, what string, fn func()) { diff --git a/internal/config/config.go b/internal/config/config.go index 1cb73d15..d7c12b70 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -311,9 +311,11 @@ type TopologyConfig struct { // each one multiplies both the index and the resync walk for no new // information — one workspace reached 20,291 files and 753 MiB this way. It // is an OPT-IN rather than an unconditional exclusion because the rule is - // exact: only a directory whose .git FILE records a gitdir under - // /.git/worktrees/ is skipped, so submodules - // (/.git/modules/) stay indexed either way. + // exact (paths.LinkedWorktreeOf): only a verified linked worktree of the + // workspace's repository or of one of its submodules is skipped, so + // submodules stay indexed either way. The OS file watcher shared by the + // index and the language servers leaves the same worktrees out; with this + // set, the index watches through a watcher of its own that keeps them. IndexWorktrees bool `toml:"index_worktrees"` // Watch enables OS-level file-system watching: any change to a source file — // by this agent, another agent, or an external editor — is re-indexed at the diff --git a/internal/fswatch/backend_darwin.go b/internal/fswatch/backend_darwin.go index 4825d46e..703cad18 100644 --- a/internal/fswatch/backend_darwin.go +++ b/internal/fswatch/backend_darwin.go @@ -76,6 +76,13 @@ func (p *fseventsPump) run(batches <-chan []rawEvent) { } } +// skipped is everything FSEvents does not filter that the watcher drops for an +// event at path: Options.ExcludeRegex and Options.SkipLinkedWorktrees. (sgtdi +// applies the regex itself; pumpEvent checks the worktrees.) +func (w *Watcher) skipped(path string, op Op) bool { + return w.excluded(path) || w.worktrees.inside(path, op) +} + // accept takes one FSEvents record through loss detection, rebasing, // exclusion, debouncing and directory expansion. func (p *fseventsPump) accept(ev rawEvent, now time.Time) { @@ -99,7 +106,7 @@ func (p *fseventsPump) accept(ev rawEvent, now time.Time) { } return } - if p.w.excluded(path) { + if p.w.skipped(path, op) { return } p.emit(path, op, now) diff --git a/internal/fswatch/backend_sgtdi.go b/internal/fswatch/backend_sgtdi.go index b59c30ca..0e4bc6b9 100644 --- a/internal/fswatch/backend_sgtdi.go +++ b/internal/fswatch/backend_sgtdi.go @@ -382,8 +382,11 @@ func (w *Watcher) pumpEvent(ev fswatcher.WatchEvent, limitReached func(dir strin // root's own metadata, with nothing for a consumer to do. return op.Has(Remove | Rename) } + if w.worktrees.inside(ev.Path, op) { + return false // sgtdi applied ExcludeRegex itself + } w.deliver(Event{Path: ev.Path, Op: op}) - if arrivedDir(ev.Path, op) { + if arrivedDir(ev.Path, op) && !w.worktrees.arrived(ev.Path) { w.expand(ev.Path, func(child string) { w.deliver(Event{Path: child, Op: Create}) }) if !w.isFailed() && limitReached(ev.Path) { w.degrade(limitMsg, nil) diff --git a/internal/fswatch/expand.go b/internal/fswatch/expand.go index 2c4ede76..085f0de8 100644 --- a/internal/fswatch/expand.go +++ b/internal/fswatch/expand.go @@ -39,7 +39,7 @@ func (w *Watcher) excluded(path string) bool { // subdirectory it cannot read) it asks for a reconcile. An entry that vanished // mid-walk is not a loss: its removal is an event of its own. func (w *Watcher) expandIfDir(path string, op Op, emit func(string)) { - if arrivedDir(path, op) { + if arrivedDir(path, op) && !w.worktrees.arrived(path) { w.expand(path, emit) } } @@ -72,7 +72,7 @@ func (w *Watcher) expand(path string, emit func(string)) { if d.IsDir() { // With the trailing separator, a pattern that excludes everything // UNDER a directory (ExcludeDirsRegex's dot branch) prunes it whole. - if w.excluded(p) || w.excluded(p+string(filepath.Separator)) { + if w.excluded(p) || w.excluded(p+string(filepath.Separator)) || w.worktrees.arrived(p) { return filepath.SkipDir } } else if w.excluded(p) { diff --git a/internal/fswatch/fswatch.go b/internal/fswatch/fswatch.go index 3a229788..99d449a9 100644 --- a/internal/fswatch/fswatch.go +++ b/internal/fswatch/fswatch.go @@ -104,6 +104,10 @@ type Options struct { // base name it matches, before debouncing. It filters events only: it never // narrows what the OS watches. Build one with ExcludeDirsRegex. ExcludeRegex string + // SkipLinkedWorktrees drops every event inside a linked worktree of the + // root (paths.LinkedWorktreeOf), and never expands one that arrives whole + // (worktrees.go). Like ExcludeRegex it filters events only. + SkipLinkedWorktrees bool } // ErrUnsupported is returned by New on a platform with no backend that is safe @@ -132,6 +136,7 @@ type Watcher struct { failed chan struct{} exclude *regexp.Regexp // Options.ExcludeRegex, nil when empty + worktrees *worktreeSkip // Options.SkipLinkedWorktrees, nil when off expandLimit int // maxExpand, smaller in tests done chan struct{} @@ -174,6 +179,9 @@ func New(root string, opts Options) (*Watcher, error) { expandLimit: maxExpand, done: make(chan struct{}), } + if opts.SkipLinkedWorktrees { + w.worktrees = newWorktreeSkip(w.root) + } if err := w.startBackend(opts); err != nil { return nil, err } @@ -230,6 +238,7 @@ func (w *Watcher) deliver(ev Event) { // signalLost records that events may have been lost. Coalescing: if a signal // is already pending, this one is folded into it. func (w *Watcher) signalLost() { + w.worktrees.lost() // what was lost may have replaced a worktree (worktrees.go) select { case w.lost <- struct{}{}: default: diff --git a/internal/fswatch/worktrees.go b/internal/fswatch/worktrees.go new file mode 100644 index 00000000..dd5e8809 --- /dev/null +++ b/internal/fswatch/worktrees.go @@ -0,0 +1,222 @@ +package fswatch + +import ( + "path/filepath" + "strings" + "sync" + "sync/atomic" + + "github.com/plumbkit/plumb/internal/paths" +) + +// worktrees.go — Options.SkipLinkedWorktrees: a watcher that drops every event +// inside a linked worktree of its root. Agents create and remove worktrees +// inside a workspace by the dozen, each a full copy of the tree. Without this, +// every checkout or removal was a burst of thousands of events (and, for a +// worktree moved in whole, an expansion of all of it) that filled the event +// buffer and reported loss: on plumb-ops, a shared watcher over the agents' +// worktrees logged 583k lost-event warnings in a day. +// +// A directory is a worktree when it verifies as one (paths.LinkedWorktreeOf) or +// when git's own bookkeeping records it (paths.LinkedWorktreesOf: the admin +// directories' back-links, read when the watcher starts and again whenever a +// .git entry changes or events are lost). The second half is what keeps a +// REMOVAL quiet: `git worktree remove` and `rm -rf` delete the checkout's .git +// along with everything else, often first, and the admin directory outlives +// the checkout. +// +// Verdicts are cached per directory, so a steady stream costs a map lookup per +// ancestor. A "worktree" verdict is dropped only by an event that creates, +// removes or renames the directory itself; a .git event re-decides a "not a +// worktree" verdict (`git worktree add` creates the directory before it writes +// .git) but never downgrades a "worktree" one mid-removal, and neither does the +// directory's own Remove. An event that creates or renames into a recorded +// worktree's path re-decides it and supersedes the record, which can be stale. +// Lost events re-decide every verdict against fresh bookkeeping, since what was +// lost may have replaced a directory. + +// worktreeCacheMax bounds the "not a worktree" verdicts; past it they are +// dropped wholesale and rebuilt lazily. "Worktree" verdicts are never evicted: +// there are few, and losing one mid-removal is the leak this file prevents. +const worktreeCacheMax = 4096 + +// worktreeSkip answers "is this path inside a linked worktree of root". +// +// Concurrency: safe for concurrent use; mu guards every field below it. +type worktreeSkip struct { + root string + + mu sync.Mutex + known map[string]bool // directory below root -> it is a linked worktree of root + recorded map[string]struct{} // worktrees git's bookkeeping records, in root's spelling + // superseded are recorded worktrees whose path an event has since created or + // renamed into: the record may be stale (an admin directory outlives an + // `rm -rf` until `git worktree prune`), so only a live verification counts + // there, until a .git that verifies appears. + superseded map[string]struct{} + + // dirty is set by lost and consumed by the next lookup, which re-reads the + // bookkeeping and re-decides every verdict once: a burst of loss signals + // costs one refresh, not one per signal. + dirty atomic.Bool + + // isWorktree and listWorktrees are paths.LinkedWorktreeOf and + // paths.LinkedWorktreesOf, seams for tests that count or fake lookups. + isWorktree func(root, dir string) bool + listWorktrees func(root string) []string +} + +func newWorktreeSkip(root string) *worktreeSkip { + s := &worktreeSkip{root: root, known: map[string]bool{}, superseded: map[string]struct{}{}, isWorktree: paths.LinkedWorktreeOf, listWorktrees: paths.LinkedWorktreesOf} + s.refreshLocked() + return s +} + +// inside reports whether path, the subject of an event with op, lies inside a +// linked worktree of root: whether a directory strictly between root and path +// is one. The root itself is never judged (a workspace that IS a linked +// worktree is watched), and nor is path itself, which may be a file; arrived +// reports on a directory that just arrived. +func (s *worktreeSkip) inside(path string, op Op) bool { + if s == nil { + return false + } + rel, ok := s.below(path) + if !ok { + return false + } + s.mu.Lock() + defer s.mu.Unlock() + s.settleLocked() + s.forgetLocked(path, op) + dir := s.root + parts := strings.Split(rel, string(filepath.Separator)) + for _, part := range parts[:len(parts)-1] { + dir = filepath.Join(dir, part) + if s.verdictLocked(dir) { + return true + } + } + return false +} + +// arrived reports whether dir, a directory that just arrived below root, is +// itself a linked worktree — which is then not expanded. +func (s *worktreeSkip) arrived(dir string) bool { + if s == nil { + return false + } + if _, ok := s.below(dir); !ok { + return false + } + s.mu.Lock() + defer s.mu.Unlock() + s.settleLocked() + // Remembered: a worktree that arrives is not expanded, so nothing inside it + // may ever be looked up before it is removed — by when its .git and git's + // record of it can both be gone. + v := s.decideLocked(dir) + if v { + s.known[dir] = true + } + return v +} + +// lost marks every verdict for re-deciding: the events that would have told +// this watcher a worktree was removed and replaced (or a directory became one) +// may be among those lost. It is O(1); the next lookup does the work. +func (s *worktreeSkip) lost() { + if s != nil { + s.dirty.Store(true) + } +} + +// settleLocked applies a pending lost: re-read the bookkeeping and drop every +// verdict. s.mu must be held. +func (s *worktreeSkip) settleLocked() { + if s.dirty.Swap(false) { + s.refreshLocked() + clear(s.known) + } +} + +// below is path relative to root, when it lies strictly below it. +func (s *worktreeSkip) below(path string) (string, bool) { + rel, err := filepath.Rel(s.root, path) + if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", false + } + return rel, true +} + +// forgetLocked drops what an event at path with op may have changed. An event +// that creates or renames into path re-decides path itself. A .git event +// re-reads the bookkeeping and re-decides the directory holding it, unless that +// directory is already a worktree: its .git going away is how a removal starts. +// s.mu must be held. +func (s *worktreeSkip) forgetLocked(path string, op Op) { + // Only a directory arriving at the path re-decides it and supersedes its + // record. Its own Remove does neither: sgtdi delivers a burst's paths in no + // set order, at least its cooldown after the fact (by when git has deleted + // the admin directory too), so the directory's Remove can come before its + // children's, and the rest of the removal must stay skipped. + if op.Has(Create | Rename) { + delete(s.known, path) + if _, ok := s.recorded[path]; ok { + s.superseded[path] = struct{}{} + } + } + if filepath.Base(path) == ".git" { + dir := filepath.Dir(path) + if dir != s.root && !s.known[dir] { // the root's own .git changes with every git command + delete(s.known, dir) + s.refreshLocked() + } + } +} + +// verdictLocked is dir's cached verdict, decided on a miss. s.mu must be held. +func (s *worktreeSkip) verdictLocked(dir string) bool { + if v, ok := s.known[dir]; ok { + return v + } + if len(s.known) >= worktreeCacheMax { + for d, v := range s.known { + if !v { + delete(s.known, d) + } + } + } + v := s.decideLocked(dir) + s.known[dir] = v + return v +} + +// decideLocked decides dir from the disk: a verified worktree, or one git's +// bookkeeping records. s.mu must be held. +func (s *worktreeSkip) decideLocked(dir string) bool { + if s.isWorktree(s.root, dir) { + delete(s.superseded, dir) // a worktree again: its record can be trusted + return true + } + _, recorded := s.recorded[dir] + _, superseded := s.superseded[dir] + return recorded && !superseded +} + +// refreshLocked re-reads git's bookkeeping, keyed in the root's own spelling +// (event paths use it; the bookkeeping is canonical). s.mu must be held. +func (s *worktreeSkip) refreshLocked() { + canonRoot := paths.Canonical(s.root) + s.recorded = map[string]struct{}{} + for _, wt := range s.listWorktrees(s.root) { + if rel, err := filepath.Rel(canonRoot, wt); err == nil { + s.recorded[filepath.Join(s.root, rel)] = struct{}{} + } + } + for d := range s.superseded { + if _, ok := s.recorded[d]; !ok { + delete(s.superseded, d) // no longer recorded: nothing to supersede + } + } +} diff --git a/internal/fswatch/worktrees_test.go b/internal/fswatch/worktrees_test.go new file mode 100644 index 00000000..25686af7 --- /dev/null +++ b/internal/fswatch/worktrees_test.go @@ -0,0 +1,524 @@ +package fswatch + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + "time" +) + +// worktrees_test.go — Options.SkipLinkedWorktrees. The layouts are built by +// hand, as git writes them (paths.LinkedWorktreeOf verifies the commondir and +// the back-link, so the fixture must carry both); paths' own tests cover the +// classification, these cover the watcher. + +// gitRepoAt makes dir a main checkout: a .git directory with HEAD and objects. +func gitRepoAt(t *testing.T, dir string) { + t.Helper() + populate(t, filepath.Join(dir, ".git"), "HEAD") + if err := os.WriteFile(filepath.Join(dir, ".git", "HEAD"), []byte("ref: refs/heads/main\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, ".git", "objects"), 0o755); err != nil { + t.Fatal(err) + } +} + +// worktreeAdmin writes the admin side of a linked worktree of the repository at +// repo whose checkout will live at wt: /.git/worktrees/ with its +// commondir and its back-link to wt/.git. +func worktreeAdmin(t *testing.T, repo, id, wt string) string { + t.Helper() + admin := filepath.Join(repo, ".git", "worktrees", id) + if err := os.MkdirAll(admin, 0o755); err != nil { + t.Fatal(err) + } + for name, body := range map[string]string{"commondir": "../..\n", "gitdir": filepath.Join(wt, ".git") + "\n"} { + if err := os.WriteFile(filepath.Join(admin, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + return admin +} + +// worktreeLink writes wt's .git file, pointing at its admin directory. +func worktreeLink(t *testing.T, wt, admin string) { + t.Helper() + if err := os.MkdirAll(wt, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(wt, ".git"), []byte("gitdir: "+admin+"\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +// eventsUnder is every reported path inside dir. +func eventsUnder(c *collector, dir string) []string { + var out []string + for _, ev := range c.snapshot() { + if strings.HasPrefix(ev.Path, dir+string(filepath.Separator)) { + out = append(out, ev.Path) + } + } + return out +} + +// rootStillReports writes a file at the root until the watcher reports it — the +// positive control that the watcher was live while the worktree was quiet. +func rootStillReports(t *testing.T, c *collector, root string) { + t.Helper() + expectEventually(t, c, filepath.Join(root, "ping.go"), "the root itself") +} + +func TestWatcher_SkipsAnExistingLinkedWorktree(t *testing.T) { + for _, skip := range []bool{true, false} { + t.Run(map[bool]string{true: "skipped", false: "control: watched"}[skip], func(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + wt := filepath.Join(root, "plumb-wt-a") + worktreeLink(t, wt, worktreeAdmin(t, root, "plumb-wt-a", wt)) + c := startWatcher(t, root, Options{SkipLinkedWorktrees: skip}) + + inside := filepath.Join(wt, "pkg", "a.go") + populate(t, wt, "pkg/a.go") + if !skip { + expectEventually(t, c, inside, "the worktree, with the skip off") + return + } + rootStillReports(t, c, root) + if err := os.WriteFile(inside, []byte("y"), 0o644); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + if got := eventsUnder(c, wt); len(got) != 0 { + t.Fatalf("events inside a linked worktree were reported: %v", got) + } + }) + } +} + +// TestWatcher_SkipsAWorktreeCreatedInPlace: `git worktree add` makes the +// directory, then writes its .git, then checks files out. The first verdict for +// the directory can be "not a worktree"; the .git write must correct it. +func TestWatcher_SkipsAWorktreeCreatedInPlace(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + + wt := filepath.Join(root, "plumb-wt-new") + early := filepath.Join(wt, "early.go") + populate(t, wt, "early.go") // before the .git exists: an ordinary directory + expectEventually(t, c, early, "a plain directory, before it is a worktree") + + worktreeLink(t, wt, worktreeAdmin(t, root, "plumb-wt-new", wt)) + rootStillReports(t, c, root) + before := len(eventsUnder(c, wt)) + populate(t, wt, "late/a.go", "late/b.go") + if err := os.WriteFile(early, []byte("again"), 0o644); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + time.Sleep(200 * time.Millisecond) + if got := eventsUnder(c, wt); len(got) != before { + t.Fatalf("after its .git was written, the worktree still reported %v", got[before:]) + } +} + +// TestWatcher_DoesNotExpandAWorktreeMovedInWhole: a populated worktree renamed +// into the tree arrives as one directory; it must not be expanded. +func TestWatcher_DoesNotExpandAWorktreeMovedInWhole(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + staging := filepath.Join(t.TempDir(), "wt") + wt := filepath.Join(root, "plumb-wt-moved") + populate(t, staging, "a.go", "deep/b.go") + worktreeLink(t, staging, worktreeAdmin(t, root, "plumb-wt-moved", wt)) // the back-link names its final place + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + + if err := os.Rename(staging, wt); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + time.Sleep(200 * time.Millisecond) + if got := eventsUnder(c, wt); len(got) != 0 { + t.Fatalf("a worktree moved in whole was expanded: %v", got) + } +} + +// fakeSkip is a worktreeSkip over /ws whose verified worktrees and recorded +// bookkeeping the test sets, counting the verifications it makes. +var listed int // fakeSkip's bookkeeping reads, across the test binary + +func fakeSkip(t *testing.T) (s *worktreeSkip, verified, recorded map[string]bool, asked map[string]int) { + t.Helper() + root := filepath.Join(string(filepath.Separator), "ws") + verified, recorded, asked = map[string]bool{}, map[string]bool{}, map[string]int{} + s = &worktreeSkip{root: root, known: map[string]bool{}, superseded: map[string]struct{}{}} + s.isWorktree = func(r, dir string) bool { + if r != root { + t.Fatalf("judged against %q, want the root", r) + } + asked[dir]++ + return verified[dir] + } + s.listWorktrees = func(string) []string { + listed++ + var out []string + for d, ok := range recorded { + if ok { + out = append(out, d) + } + } + return out + } + s.refreshLocked() + return s, verified, recorded, asked +} + +func TestWorktreeSkip_CachesAndForgets(t *testing.T) { + s, verified, _, asked := fakeSkip(t) + root := s.root + at := func(rel string) string { return filepath.Join(root, filepath.FromSlash(rel)) } + in := func(rel string, op Op) bool { return s.inside(at(rel), op) } + verified[at("wt")] = true + + if !in("wt/a/b.go", Write) || !in("wt/c.go", Write) { + t.Fatal("paths inside the worktree were not skipped") + } + if asked[at("wt")] != 1 { + t.Errorf("the worktree was judged %d times, want once (cached)", asked[at("wt")]) + } + if in("src/x.go", Write) || in("top.go", Write) { + t.Fatal("paths outside any worktree were skipped") + } + if _, judged := asked[at("top.go")]; judged { + t.Error("an event's own path was judged: it may be a file") + } + if _, judged := asked[root]; judged { + t.Error("the root was judged") + } + + // A directory that becomes a worktree: its .git event re-decides it. + verified[at("src")] = true + if in("src/y.go", Write) { + t.Fatal("control: the cached verdict stands until something says otherwise") + } + in("src/.git", Create) + if !in("src/y.go", Write) { + t.Error("a .git event did not re-decide its directory") + } + // Its .git going away (a removal starting) does not downgrade it. + verified[at("src")] = false + in("src/.git", Remove) + if !in("src/z.go", Remove) { + t.Error("a worktree's .git removal downgraded it mid-removal") + } + // A metadata event for the directory itself re-decides nothing; its own + // removal does. + in("src", Chmod) + if !in("src/z.go", Remove) { + t.Error("a metadata event for the worktree re-decided it") + } + // Nor does its own Remove (it can arrive before its children's); a + // directory arriving at the path does. + in("src", Remove) + if !in("src/z.go", Remove) { + t.Error("the worktree's own Remove re-decided it mid-removal") + } + in("src", Create) + if in("src/z.go", Create) { + t.Error("a directory arriving at the path did not re-decide it") + } + + var off *worktreeSkip + if off.inside(at("wt/a.go"), Write) || off.arrived(at("wt")) { + t.Error("a watcher without the option skipped something") + } + off.lost() + if s.arrived(root) || s.inside(filepath.Join(string(filepath.Separator), "elsewhere", "wt", "a.go"), Write) { + t.Error("the root itself, or a path outside it, was judged") + } +} + +// TestWorktreeSkip_BookkeepingCoversARemoval: a worktree whose .git is already +// gone (removed first, or before this watcher ever saw it) is still a worktree +// while git's bookkeeping records it. +func TestWorktreeSkip_BookkeepingCoversARemoval(t *testing.T) { + s, _, recorded, _ := fakeSkip(t) + wt := filepath.Join(s.root, "plumb-wt-gone") + recorded[wt] = true + s.lost() // re-read the bookkeeping + if !s.inside(filepath.Join(wt, "a.go"), Remove) || !s.arrived(wt) { + t.Fatal("a recorded worktree with no .git was not skipped") + } + // Once the bookkeeping no longer records it and events were lost, the + // directory is re-decided: it may have been replaced by an ordinary one. + recorded[wt] = false + s.lost() + if s.inside(filepath.Join(wt, "a.go"), Create) { + t.Error("after a loss, a directory git no longer records stayed skipped") + } +} + +// TestWorktreeSkip_TheCapKeepsWorktrees: a full cache drops "not a worktree" +// verdicts, never a "worktree" one. +func TestWorktreeSkip_TheCapKeepsWorktrees(t *testing.T) { + s, verified, _, asked := fakeSkip(t) + wt := filepath.Join(s.root, "wt") + verified[wt] = true + s.inside(filepath.Join(wt, "a.go"), Write) + for i := range worktreeCacheMax + 10 { + s.inside(filepath.Join(s.root, "d"+strconv.Itoa(i), "x.go"), Write) + } + verified[wt] = false // a re-decision now would say no + if !s.inside(filepath.Join(wt, "b.go"), Write) || asked[wt] != 1 { + t.Errorf("the cap evicted a worktree verdict (judged %d times)", asked[wt]) + } + s.mu.Lock() + n := len(s.known) + s.mu.Unlock() + if n > worktreeCacheMax+1 { + t.Errorf("the cache holds %d verdicts, past its cap", n) + } +} + +// TestWatcher_ExpansionPrunesANestedWorktree: a directory moved in whole that +// holds a worktree is expanded, but not into the worktree. +func TestWatcher_ExpansionPrunesANestedWorktree(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + staging := filepath.Join(t.TempDir(), "agents") + moved := filepath.Join(root, "agents") + wt := filepath.Join(moved, "wt") + populate(t, staging, "notes.go", "wt/a.go", "wt/deep/b.go") + worktreeLink(t, filepath.Join(staging, "wt"), worktreeAdmin(t, root, "wt", wt)) + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + + if err := os.Rename(staging, moved); err != nil { + t.Fatal(err) + } + notes := filepath.Join(moved, "notes.go") + if !waitFor(func() bool { return c.count(notes, 0) > 0 }, 15*time.Second) { + t.Fatalf("control: the moved directory's own file was not reported; got %v", c.snapshot()) + } + rootStillReports(t, c, root) + if got := eventsUnder(c, wt); len(got) != 0 { + t.Fatalf("the expansion walked into a nested worktree: %v", got) + } +} + +// TestWatcher_ARemovalIsQuiet: deleting a worktree — warm (seen before), cold +// (never seen), or with its .git deleted first — reports nothing inside it +// while git's admin directory still records it, which is the order both +// `git worktree remove` and `rm -rf` leave things in. +func TestWatcher_ARemovalIsQuiet(t *testing.T) { + for _, tc := range []struct { + name string + warm bool + gitsFirst bool + }{{"warm", true, false}, {"cold", false, false}, {".git first", false, true}} { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + wt := filepath.Join(root, "plumb-wt-rm") + populate(t, wt, "a.go", "pkg/b.go", "pkg/deep/c.go") + worktreeLink(t, wt, worktreeAdmin(t, root, "plumb-wt-rm", wt)) + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + if tc.warm { + if err := os.WriteFile(filepath.Join(wt, "a.go"), []byte("w"), 0o644); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + } + if tc.gitsFirst { + if err := os.Remove(filepath.Join(wt, ".git")); err != nil { + t.Fatal(err) + } + } + if err := os.RemoveAll(wt); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + time.Sleep(200 * time.Millisecond) + if got := eventsUnder(c, wt); len(got) != 0 { + t.Fatalf("a worktree's removal was reported: %v", got) + } + }) + } +} + +// TestWatcher_GitWorktreeAddAndRemoveAreQuiet runs real git. +func TestWatcher_GitWorktreeAddAndRemoveAreQuiet(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + root := t.TempDir() + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", root, "-c", "user.email=t@t", "-c", "user.name=t", "-c", "commit.gpgsign=false"}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + git("init", "-q", "-b", "main") + populate(t, root, "pkg/a.go", "pkg/deep/b.go", "c.go") + git("add", ".") + git("commit", "-q", "-m", "init") + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + + wt := filepath.Join(root, "plumb-wt-git") + git("worktree", "add", "-q", wt) + rootStillReports(t, c, root) + git("worktree", "remove", "--force", wt) + rootStillReports(t, c, root) + time.Sleep(200 * time.Millisecond) + if got := eventsUnder(c, wt); len(got) > 0 { + t.Fatalf("git worktree add/remove reported %d events inside the worktree: %v", len(got), got) + } +} + +// TestWorktreeSkip_AGitEventRereadsTheBookkeeping: a worktree git records +// before its checkout verifies (commondir not yet written) is found when its +// .git appears. +func TestWorktreeSkip_AGitEventRereadsTheBookkeeping(t *testing.T) { + s, _, recorded, _ := fakeSkip(t) + wt := filepath.Join(s.root, "plumb-wt-new") + if s.inside(filepath.Join(wt, "a.go"), Create) { + t.Fatal("control: an unrecorded, unverified directory was skipped") + } + recorded[wt] = true + s.inside(filepath.Join(wt, ".git"), Create) + if !s.inside(filepath.Join(wt, "b.go"), Create) { + t.Error("the .git event did not re-read git's bookkeeping") + } +} + +// TestWatcher_LostEventsRedecideWorktrees: the watcher's loss signal reaches +// the worktree verdicts. +func TestWatcher_LostEventsRedecideWorktrees(t *testing.T) { + root := t.TempDir() + w, err := New(root, Options{SkipLinkedWorktrees: true}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(w.Close) + w.worktrees.inside(filepath.Join(root, "d", "x.go"), Write) + w.signalLost() + if !w.worktrees.dirty.Load() { + t.Fatal("lost events did not mark the worktree verdicts for re-deciding") + } +} + +// TestWorktreeSkip_ARecreatedPathOutlivesItsStaleRecord: an ordinary +// directory created where a removed worktree was is watched, though git's +// bookkeeping still records the worktree (an `rm -rf` leaves the admin +// directory until `git worktree prune`) — and stays watched across a later +// re-read of that bookkeeping. +func TestWorktreeSkip_ARecreatedPathOutlivesItsStaleRecord(t *testing.T) { + s, verified, recorded, _ := fakeSkip(t) + wt := filepath.Join(s.root, "plumb-wt-old") + recorded[wt] = true + s.lost() + if !s.inside(filepath.Join(wt, "a.go"), Remove) { + t.Fatal("control: the recorded worktree's removal was not skipped") + } + s.inside(wt, Remove) + s.inside(wt, Create) // an ordinary directory at the same path + if s.inside(filepath.Join(wt, "b.go"), Create) { + t.Fatal("an ordinary directory at a removed worktree's path was skipped on a stale record") + } + s.inside(filepath.Join(s.root, "other", ".git"), Create) // re-reads the bookkeeping + s.lost() + if s.inside(filepath.Join(wt, "c.go"), Write) { + t.Error("a re-read of the bookkeeping brought the stale record back") + } + // A worktree created there again is one. + verified[wt] = true + s.inside(filepath.Join(wt, ".git"), Create) + if !s.inside(filepath.Join(wt, "d.go"), Create) { + t.Error("a new worktree at the path was not skipped") + } + // Its removal, after a loss (a cold verdict), is covered by its record + // again: verifying it as a worktree lifted the supersede. + verified[wt] = false + s.lost() + if !s.inside(filepath.Join(wt, "e.go"), Remove) { + t.Error("the new worktree's removal fell back on a superseded record") + } +} + +// TestWorktreeSkip_LostIsLazy: a burst of loss signals costs one re-read, at +// the next lookup; the root's own .git changing costs none. +func TestWorktreeSkip_LostIsLazy(t *testing.T) { + s, _, _, _ := fakeSkip(t) + before := listed + for range 100 { + s.lost() + } + if listed != before { + t.Fatalf("lost re-read the bookkeeping %d times by itself", listed-before) + } + s.inside(filepath.Join(s.root, "a", "b.go"), Write) + s.inside(filepath.Join(s.root, "a", "c.go"), Write) + if listed-before != 1 { + t.Fatalf("a burst of losses re-read the bookkeeping %d times, want 1", listed-before) + } + s.inside(filepath.Join(s.root, ".git"), Write) + if listed-before != 1 { + t.Error("the root's own .git changing re-read the bookkeeping") + } +} + +// TestWatcher_AnOrdinaryDirectoryWhereAWorktreeWasIsWatched: the real-backend +// shape of the stale-record case: `rm -rf` of a worktree (its admin directory +// stays), then an ordinary directory at the same path. +func TestWatcher_AnOrdinaryDirectoryWhereAWorktreeWasIsWatched(t *testing.T) { + root := t.TempDir() + gitRepoAt(t, root) + wt := filepath.Join(root, "plumb-wt-reused") + populate(t, wt, "a.go") + worktreeLink(t, wt, worktreeAdmin(t, root, "plumb-wt-reused", wt)) + c := startWatcher(t, root, Options{SkipLinkedWorktrees: true}) + if err := os.RemoveAll(wt); err != nil { + t.Fatal(err) + } + rootStillReports(t, c, root) + populate(t, wt, "plain.go") + expectEventually(t, c, filepath.Join(wt, "plain.go"), "an ordinary directory where a worktree was") +} + +// TestWorktreeSkip_ADirectorysOwnRemoveFirstKeepsItsRecord: sgtdi can deliver +// a removed worktree's own Remove before its children's; those stay skipped. +func TestWorktreeSkip_ADirectorysOwnRemoveFirstKeepsItsRecord(t *testing.T) { + s, _, recorded, _ := fakeSkip(t) + wt := filepath.Join(s.root, "plumb-wt-gone") + recorded[wt] = true + s.lost() + s.inside(wt, Remove) + for _, rel := range []string{"a.go", "pkg/b.go"} { + if !s.inside(filepath.Join(wt, filepath.FromSlash(rel)), Remove) { + t.Errorf("%s's removal, after the directory's own, was reported", rel) + } + } +} + +// TestWorktreeSkip_AnArrivedWorktreeIsRemembered: a worktree seen only by its +// own arrival (never expanded, nothing inside it looked up) stays skipped +// through a removal processed after git has forgotten it. +func TestWorktreeSkip_AnArrivedWorktreeIsRemembered(t *testing.T) { + s, verified, _, _ := fakeSkip(t) + wt := filepath.Join(s.root, "plumb-wt-brief") + verified[wt] = true + s.inside(wt, Create) + if !s.arrived(wt) { + t.Fatal("control: the arrived worktree was not recognised") + } + verified[wt] = false // its .git and git's record are gone by now + s.inside(wt, Remove) + if !s.inside(filepath.Join(wt, "pkg"), Remove) { + t.Error("a worktree seen only arriving leaked its removal") + } +} diff --git a/internal/paths/checkout_worktree_test.go b/internal/paths/checkout_worktree_test.go new file mode 100644 index 00000000..8242c97b --- /dev/null +++ b/internal/paths/checkout_worktree_test.go @@ -0,0 +1,109 @@ +package paths + +import ( + "os" + "path/filepath" + "testing" +) + +// TestLinkedWorktreeOf covers both answers: the checkouts the topology index +// and the file watcher leave out (a second checkout of code root already +// holds) and the ones they must keep. +func TestLinkedWorktreeOf(t *testing.T) { + root := mainCheckout(t, t.TempDir()) + common := filepath.Join(root, ".git") + + ownWT := filepath.Join(root, "plumb-wt-a") + linkedWorktree(t, common, "plumb-wt-a", ownWT, false) + + sub := filepath.Join(root, "plumb") + subGit := submodule(t, root, "plumb", sub, "../../../plumb") + subWT := filepath.Join(root, "plumb-wt-b") + linkedWorktree(t, subGit, "plumb-wt-b", subWT, true) + + // A submodule at a path spelling the worktree shape is still a submodule. + shaped := filepath.Join(root, "libs", "worktrees", "foo") + submodule(t, root, "libs/worktrees/foo", shaped, "../../../../../libs/worktrees/foo") + + // An unrelated repository living under root, and a worktree of it. + other := mainCheckout(t, filepath.Join(root, "repos", "other")) + otherWT := filepath.Join(root, "repos", "other-wt") + linkedWorktree(t, filepath.Join(other, ".git"), "other-wt", otherWT, false) + + // A hand-written pointer in the worktree shape that git's bookkeeping + // does not back. + forged := filepath.Join(root, "forged") + write(t, filepath.Join(forged, ".git"), "gitdir: "+filepath.Join(common, "worktrees", "forged")+"\n") + + for _, tc := range []struct { + name string + dir string + want bool + }{ + {"a worktree of the root repository", ownWT, true}, + {"a worktree of a submodule of root", subWT, true}, + {"the submodule itself", sub, false}, + {"a submodule whose path holds a worktrees segment", shaped, false}, + {"an unrelated repository under root", other, false}, + {"a worktree of an unrelated repository under root", otherWT, false}, + {"a forged worktree pointer", forged, false}, + {"a plain directory", filepath.Join(root, "repos"), false}, + {"the root itself", root, false}, + } { + if got := LinkedWorktreeOf(root, tc.dir); got != tc.want { + t.Errorf("%s: LinkedWorktreeOf = %v, want %v", tc.name, got, tc.want) + } + } +} + +// TestLinkedWorktreeOf_NeedsARepositoryRoot: under a root that is no checkout +// nothing is anybody's worktree. +func TestLinkedWorktreeOf_NeedsARepositoryRoot(t *testing.T) { + plain := t.TempDir() + repo := mainCheckout(t, filepath.Join(plain, "repo")) + wt := filepath.Join(plain, "wt") + linkedWorktree(t, filepath.Join(repo, ".git"), "wt", wt, false) + if LinkedWorktreeOf(plain, wt) { + t.Error("a worktree was classified against a root that is not a checkout") + } + if !LinkedWorktreeOf(repo, wt) { + t.Error("control: the worktree is its repository's, when that repository is the root") + } +} + +// TestLinkedWorktreesOf lists git's own records of root's worktrees and its +// submodules' — including one whose checkout has lost its .git (a removal in +// progress) — and nothing outside root. +func TestLinkedWorktreesOf(t *testing.T) { + root := mainCheckout(t, t.TempDir()) + common := filepath.Join(root, ".git") + ownWT := filepath.Join(root, "plumb-wt-a") + linkedWorktree(t, common, "plumb-wt-a", ownWT, false) + sub := filepath.Join(root, "libs", "plumb") + subGit := submodule(t, root, "libs/plumb", sub, "../../../../libs/plumb") + subWT := filepath.Join(root, "plumb-wt-b") + linkedWorktree(t, subGit, "plumb-wt-b", subWT, true) + removing := filepath.Join(root, "plumb-wt-c") + linkedWorktree(t, common, "plumb-wt-c", removing, false) + if err := os.Remove(filepath.Join(removing, ".git")); err != nil { + t.Fatal(err) + } + outside := filepath.Join(t.TempDir(), "elsewhere") + linkedWorktree(t, common, "elsewhere", outside, false) + + got := map[string]bool{} + for _, wt := range LinkedWorktreesOf(root) { + got[wt] = true + } + for _, want := range []string{ownWT, subWT, removing} { + if !got[Canonical(want)] { + t.Errorf("LinkedWorktreesOf = %v, missing %s", got, want) + } + } + if got[Canonical(outside)] || got[Canonical(sub)] || len(got) != 3 { + t.Errorf("LinkedWorktreesOf = %v, want exactly the three worktrees under root", got) + } + if LinkedWorktreesOf(t.TempDir()) != nil { + t.Error("a root that is no checkout records worktrees") + } +} diff --git a/internal/paths/worktrees.go b/internal/paths/worktrees.go new file mode 100644 index 00000000..6388b0b5 --- /dev/null +++ b/internal/paths/worktrees.go @@ -0,0 +1,111 @@ +package paths + +import ( + "os" + "path/filepath" + "strings" +) + +// worktrees.go answers which directories under a workspace are linked worktrees +// of it (`git worktree add`): second checkouts of code the root already holds, +// which the topology index and the file watcher leave out. Agents create and +// remove them by the dozen, and each is a full copy of the tree. +// +// Concurrency: stateless and read-only; safe for concurrent use. + +// LinkedWorktreeOf reports whether dir is a verified linked worktree of the +// repository checked out at root, or of one of root's submodules. +// +// Everything is verified the way CheckoutAt verifies it — commondir plus git's +// own back-link — so a hand-written `.git` file proves nothing and is not +// skipped. A submodule checkout (no commondir) is content the root contains and +// is never a worktree here, whatever its path is called. A worktree of an +// unrelated repository that happens to live under root is not root's either. +func LinkedWorktreeOf(root, dir string) bool { + // The cheap miss first: most directories hold no .git at all, and the + // verification below resolves symlinks. + if _, err := os.Lstat(filepath.Join(dir, ".git")); err != nil { + return false + } + c, ok := CheckoutAt(dir) + if !ok || samePath(c.GitDir, c.CommonDir) { + return false // no verified checkout, a main checkout, or a submodule + } + r, ok := CheckoutAt(root) + if !ok { + return false + } + return samePath(c.CommonDir, r.CommonDir) || nestedGitDir(c.CommonDir, r.CommonDir) +} + +// LinkedWorktreesOf lists the checkouts git's own bookkeeping records as linked +// worktrees of root's repository and of its submodules: every +// /worktrees//gitdir back-link, read from the repository side. It +// lists a worktree whose checkout is being deleted, too — `git worktree remove` +// deletes the checkout before its admin directory, and a plain `rm -rf` leaves +// the admin directory until `git worktree prune` — which a check of the +// checkout's own .git cannot, once that file is gone. Paths are canonical. +// Entries that are not directories below root are left out. +func LinkedWorktreesOf(root string) []string { + r, ok := CheckoutAt(root) + if !ok { + return nil + } + canonRoot := Canonical(root) + var out []string + for _, gitDir := range repositoryGitDirs(r.CommonDir) { + admins, err := os.ReadDir(filepath.Join(gitDir, "worktrees")) + if err != nil { + continue + } + for _, a := range admins { + if !a.IsDir() { + continue + } + admin := filepath.Join(gitDir, "worktrees", a.Name()) + data, err := os.ReadFile(filepath.Join(admin, "gitdir")) + if err != nil { + continue + } + link := strings.TrimSpace(string(data)) + if link == "" { + continue + } + wt := Canonical(filepath.Dir(resolveAgainst(admin, link))) + if rel, err := filepath.Rel(canonRoot, wt); err == nil && rel != "." && !strings.HasPrefix(rel, "..") { + out = append(out, wt) + } + } + } + return out +} + +// repositoryGitDirs is common plus every submodule git directory git keeps +// beneath it (/modules/, nested submodules in turn). A submodule's +// name may contain '/', so a directory under modules/ that is not itself a git +// directory is a name component to descend through. Nothing else in a git +// directory (objects, refs, logs) is walked. +func repositoryGitDirs(common string) []string { + out := []string{common} + var walk func(dir string) + walk = func(dir string) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, e := range entries { + if !e.IsDir() { + continue + } + child := filepath.Join(dir, e.Name()) + if isGitDir(child) { + out = append(out, child) + walk(filepath.Join(child, "modules")) + } else { + walk(child) + } + } + } + walk(filepath.Join(common, "modules")) + return out +} diff --git a/internal/topology/indexer.go b/internal/topology/indexer.go index ca76f5e9..0200d1e6 100644 --- a/internal/topology/indexer.go +++ b/internal/topology/indexer.go @@ -38,12 +38,12 @@ type Indexer struct { excludePatterns []string // indexWorktrees is [topology] index_worktrees. False (the default) makes the - // resync walk prune a LINKED WORKTREE of this workspace's own repository: the - // directory holds a .git FILE whose gitdir points under /.git/ - // worktrees/, so it is a second full copy of the tree rather than another - // tree. Submodules (a .git file pointing under /.git/modules/) are - // never pruned by this rule, and neither is anything the rule cannot read: - // see linkedWorktree. Read by the resync walk; set once before Start and not + // resync walk prune a LINKED WORKTREE of this workspace's repository or of one + // of its submodules — a second full copy of the tree rather than another tree + // — as verified by paths.LinkedWorktreeOf (commondir plus git's back-link). + // Submodules are never pruned by this rule, and neither is anything it cannot + // verify. The shared file watcher leaves the same worktrees out + // (watchOptions). Read by the resync walk; set once before Start and not // mutated afterwards. indexWorktrees bool diff --git a/internal/topology/indexer_resync.go b/internal/topology/indexer_resync.go index fbcd5037..d4649a1e 100644 --- a/internal/topology/indexer_resync.go +++ b/internal/topology/indexer_resync.go @@ -7,9 +7,7 @@ import ( "fmt" "io/fs" "log/slog" - "os" "path/filepath" - "strings" "time" "github.com/plumbkit/plumb/internal/ignore" @@ -217,97 +215,16 @@ func (idx *Indexer) resyncEnterDir(root, path, name string, stacks map[string]ig return fs.SkipDir } // Last because it is the only check here that touches the filesystem: one - // os.ReadFile per surviving directory. It must still be decided before the - // stack for the directory is loaded, since the prune path never loads one. - if !idx.indexWorktrees && linkedWorktree(root, path) { + // os.Lstat per surviving directory (more only where a .git exists). It must + // still be decided before the stack for the directory is loaded, since the + // prune path never loads one. + if !idx.indexWorktrees && paths.LinkedWorktreeOf(root, path) { return fs.SkipDir } stacks[path] = parent.Load(path) return nil } -// linkedWorktree reports whether dir is a LINKED WORKTREE of the repository -// rooted at root — a second checkout of the same repository, created by -// `git worktree add` and materialised in this tree as an untracked directory -// holding a .git FILE. Nothing about the directory's NAME is consulted: the -// distinction is git's own bookkeeping, because the names are convention -// (plumb-wt-*, plumb-review-*) and a rename must not change the answer. -// -// The rule is exact, and it is deliberately silent when it cannot be exact: -// -// - a directory with no .git file is not a worktree (an ordinary directory, or -// the main checkout, whose .git is a directory); -// - a .git file whose gitdir: line points into a repository's worktrees directory -// IS a linked worktree, and is skipped unless [topology] index_worktrees opts in. -// The repository may be the workspace root OR one inside it: agent worktrees in a -// workspace whose code lives in a submodule are worktrees of THAT repository, so -// their gitdir is /.git/modules//worktrees/ — matching only -// /.git/worktrees missed every one of them (review round 1, B1); -// - a .git file pointing under /.git/modules/ WITHOUT a worktrees component is -// a SUBMODULE, whose files are part of this tree and stay indexed — this rule never -// prunes one; -// - anything else — an unreadable file, a missing gitdir: line, a path outside this -// workspace (a worktree of a DIFFERENT repository vendored in by hand) — is not -// skipped. The walk does not guess. -// -// The gitdir: target may be relative (git writes a submodule's as -// ../.git/modules/) and either side may be spelled through a symlink -// (/var vs /private/var on macOS, where git records the path it was handed), so -// both are resolved before comparison. -func linkedWorktree(root, dir string) bool { - data, err := os.ReadFile(filepath.Join(dir, ".git")) - if err != nil { - return false - } - target, ok := gitdirTarget(data) - if !ok { - return false - } - if !filepath.IsAbs(target) { - target = filepath.Join(dir, target) - } - target = paths.Canonical(filepath.Clean(target)) - // The test is the SHAPE of the part of target INSIDE this workspace: a relative path whose - // components include both .git and worktrees. Two things make the relative form load-bearing - // rather than cosmetic. First, confinement: a repository outside the workspace is not ours - // to classify, so it is never pruned on this rule. Second — and this is review round 1's B3 — - // testing the ABSOLUTE path's components would classify every file in a workspace that merely - // LIVES under a directory called "worktrees" (…/code/worktrees/myproject) as a linked - // worktree, and prune the whole workspace. Only the path below the root says anything about - // what is inside it. - rel, err := filepath.Rel(paths.Canonical(root), target) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return false - } - // git writes exactly two shapes for a worktree's admin dir, and this tests those shapes - // rather than searching for components: `/.git/worktrees/` and - // `/.git/modules//worktrees/`. A component search was wrong twice over - // (review round 1, B3 and SHOULD-FIX 2): it also matched a workspace that merely LIVES under - // a directory called "worktrees", and a SUBMODULE whose path ends in `worktrees` - // (`.git/modules/worktrees` has both components and is not a worktree at all). - if first, _, _ := strings.Cut(rel, string(filepath.Separator)); first != ".git" { - return false - } - return filepath.Base(filepath.Dir(target)) == "worktrees" -} - -// gitdirTarget extracts the path from a .git file's contents. A .git file holds -// one line, `gitdir: `; git writes nothing else, and a file that does not -// match that shape is not a gitdir pointer at all (ok is false, and the caller -// must not skip on a guess). -func gitdirTarget(data []byte) (string, bool) { - line, _, _ := strings.Cut(string(data), "\n") - target, found := strings.CutPrefix(strings.TrimSpace(line), "gitdir:") - if !found { - return "", false - } - target = strings.TrimSpace(target) - if target == "" { - return "", false - } - return target, true -} - // resyncSkipsFile reports whether the walk excludes one file. Its directory's // stack is already loaded, because a pruned directory never reaches here. func (idx *Indexer) resyncSkipsFile(path, rel string, stacks map[string]ignore.Stack) bool { diff --git a/internal/topology/indexer_resync_worktree_test.go b/internal/topology/indexer_resync_worktree_test.go index 61921e16..91fb3c5c 100644 --- a/internal/topology/indexer_resync_worktree_test.go +++ b/internal/topology/indexer_resync_worktree_test.go @@ -168,18 +168,17 @@ func TestResync_DoesNotGuessAtUnexpectedGitFile(t *testing.T) { "malformed/main.go", "modules/main.go", "empty/main.go", + // A pointer in the worktree SHAPE that git's own bookkeeping does not + // back (no admin dir, no back-link) is a claim, not a worktree: it is + // indexed (paths.LinkedWorktreeOf verifies; relative links are + // paths.CheckoutAt's, tested there). + "relative/main.go", } { if !slices.Contains(got, want) { t.Errorf("indexed = %v, want %s — the walk must not guess that a .git file "+ "it cannot classify is a worktree", got, want) } } - // The relative form of the worktree pointer is the same rule, resolved - // against the directory holding the .git file. - if under := pathsUnder(got, "relative"); len(under) != 0 { - t.Errorf("indexed %v under a .git file whose gitdir names "+ - "/.git/worktrees/relative; want none", under) - } } // TestResync_SkipsASubmoduleWorktree is the shape review round 1 caught (B1), and the case the diff --git a/internal/topology/store.go b/internal/topology/store.go index 9d6244d1..25badccc 100644 --- a/internal/topology/store.go +++ b/internal/topology/store.go @@ -60,7 +60,7 @@ func OpenWith(workspace string, cfg config.TopologyConfig, exts []Extractor, hub var watcher *fsWatcher if cfg.Watch { - w, werr := newFSWatcher(workspace, s, excludes, hub) + w, werr := newFSWatcher(workspace, s, excludes, cfg.IndexWorktrees, hub) switch { case werr != nil && cfg.ResyncIntervalMinutes == 0: // Both freshness paths are off: the index refreshes only at startup diff --git a/internal/topology/watcher.go b/internal/topology/watcher.go index f439e58b..d3eef3d8 100644 --- a/internal/topology/watcher.go +++ b/internal/topology/watcher.go @@ -121,16 +121,24 @@ type fsWatcher struct { // share through an fswatch.Hub (PLAN-503): the topology index's cooldown and // exclusions, which are the set every consumer excludes. A consumer that // excludes more (the language servers' target/ and out/) filters them on its -// own subscription. +// own subscription. The shared watcher also leaves out the root's linked +// worktrees (PLAN-491); the index's [topology] index_worktrees opt-in watches +// with watchOptions(workspace, true), a watcher of its own. func SharedWatchOptions(workspace string) fswatch.Options { - return fswatch.Options{Cooldown: watchCooldown, ExcludeRegex: watchExcludeRegexFor(workspace)} + return watchOptions(workspace, false) +} + +// watchOptions are SharedWatchOptions, with the linked worktrees watched too +// when indexWorktrees is set. +func watchOptions(workspace string, indexWorktrees bool) fswatch.Options { + return fswatch.Options{Cooldown: watchCooldown, ExcludeRegex: watchExcludeRegexFor(workspace), SkipLinkedWorktrees: !indexWorktrees} } // newFSWatcher constructs a watcher rooted at workspace. It does not start // watching until Start is called. An error here (e.g. the platform watcher // cannot be created) lets the caller fall back to periodic resync. -func newFSWatcher(workspace string, sink indexSink, excludePatterns []string, hub *fswatch.Hub) (*fsWatcher, error) { - w, err := hub.Subscribe(workspace, SharedWatchOptions(workspace), "") +func newFSWatcher(workspace string, sink indexSink, excludePatterns []string, indexWorktrees bool, hub *fswatch.Hub) (*fsWatcher, error) { + w, err := hub.Subscribe(workspace, watchOptions(workspace, indexWorktrees), "") if err != nil { return nil, err } diff --git a/internal/topology/watcher_alias_test.go b/internal/topology/watcher_alias_test.go index 6c264213..9525c6b6 100644 --- a/internal/topology/watcher_alias_test.go +++ b/internal/topology/watcher_alias_test.go @@ -7,6 +7,9 @@ import ( "slices" "testing" "time" + + "github.com/plumbkit/plumb/internal/config" + "github.com/plumbkit/plumb/internal/fswatch" ) // TestFSWatcher_NewSymlinkIsEnqueuedUnderItsOwnName: the indexer keeps a @@ -28,7 +31,7 @@ func TestFSWatcher_NewSymlinkIsEnqueuedUnderItsOwnName(t *testing.T) { t.Fatal(err) } sink := &recordingSink{} - fw, err := newFSWatcher(ws, sink, nil, nil) + fw, err := newFSWatcher(ws, sink, nil, false, nil) if err != nil { t.Fatalf("newFSWatcher: %v", err) } @@ -65,3 +68,45 @@ func TestFSWatcher_NewSymlinkIsEnqueuedUnderItsOwnName(t *testing.T) { t.Errorf("a renamed symlink was never enqueued under its new name; enqueued %v", enq) } } + +// TestWatchOptions_LeaveLinkedWorktreesOutUnlessIndexed: the watcher the index +// shares with the language servers skips linked worktrees; the +// index_worktrees opt-in watches them on options of its own. +func TestWatchOptions_LeaveLinkedWorktreesOutUnlessIndexed(t *testing.T) { + ws := t.TempDir() + if !SharedWatchOptions(ws).SkipLinkedWorktrees || !watchOptions(ws, false).SkipLinkedWorktrees { + t.Error("the shared watcher watches linked worktrees") + } + if watchOptions(ws, true).SkipLinkedWorktrees { + t.Error("index_worktrees did not keep the worktrees' events") + } + if watchOptions(ws, false) != SharedWatchOptions(ws) { + t.Error("the default index watcher does not share the language servers' options") + } +} + +// TestOpenWith_IndexWorktreesWatchesOnItsOwnOptions: a store that indexes +// worktrees does not join the shared watcher that leaves them out; one that +// does not, does. +func TestOpenWith_IndexWorktreesWatchesOnItsOwnOptions(t *testing.T) { + for _, tc := range []struct { + indexWorktrees bool + watches int + }{{false, 1}, {true, 2}} { + ws := t.TempDir() + hub := fswatch.NewHub() + s, err := OpenWith(ws, config.TopologyConfig{MaxFileSizeBytes: 512 * 1024, Watch: true, IndexWorktrees: tc.indexWorktrees}, nil, hub) + if err != nil { + t.Fatal(err) + } + lsp, err := hub.Subscribe(ws, SharedWatchOptions(ws), "") + if err != nil { + t.Fatal(err) + } + if got := hub.Watches(); got != tc.watches { + t.Errorf("index_worktrees=%v: %d OS watchers beside the shared subscriber, want %d", tc.indexWorktrees, got, tc.watches) + } + lsp.Close() + _ = s.Close() + } +} diff --git a/internal/topology/watcher_ignore_test.go b/internal/topology/watcher_ignore_test.go index a4452b48..fe159687 100644 --- a/internal/topology/watcher_ignore_test.go +++ b/internal/topology/watcher_ignore_test.go @@ -121,7 +121,7 @@ func TestFSWatcher_ThroughOSWatcher(t *testing.T) { } sink := &recordingSink{} - fw, err := newFSWatcher(ws, sink, nil, nil) + fw, err := newFSWatcher(ws, sink, nil, false, nil) if err != nil { t.Fatalf("newFSWatcher: %v", err) } diff --git a/internal/topology/watcher_lock_test.go b/internal/topology/watcher_lock_test.go index 73661bc9..21759551 100644 --- a/internal/topology/watcher_lock_test.go +++ b/internal/topology/watcher_lock_test.go @@ -162,7 +162,7 @@ func TestFSWatcher_StopKeepsSQLiteLocksInPlumbDir(t *testing.T) { } sink := &recordingSink{} - fw, err := newFSWatcher(ws, sink, nil, nil) + fw, err := newFSWatcher(ws, sink, nil, false, nil) if err != nil { t.Fatalf("newFSWatcher: %v", err) } diff --git a/internal/topology/watcher_test.go b/internal/topology/watcher_test.go index 20719fb9..922a364b 100644 --- a/internal/topology/watcher_test.go +++ b/internal/topology/watcher_test.go @@ -162,7 +162,7 @@ func (f *lockedFakeSink) resyncCount() int { // TestFSWatcher_StartStop exercises the real platform watcher's lifecycle: it // must construct, start, and stop cleanly without hanging. func TestFSWatcher_StartStop(t *testing.T) { - fw, err := newFSWatcher(t.TempDir(), &fakeSink{}, nil, nil) + fw, err := newFSWatcher(t.TempDir(), &fakeSink{}, nil, false, nil) if err != nil { t.Fatalf("newFSWatcher: %v", err) }