feat(pool): seed worktrees with ignored files - #48
Conversation
mbrookson
left a comment
There was a problem hiding this comment.
Nice implementation — using git ls-files + check-ignore for pattern matching is the right call over rolling our own glob expansion. A few observations from independently building the same feature (closed #50 in favour of this):
Seeding runs inside the state lock
Both call sites invoke SeedWorktree inside the WithStateLock closure, which means the flock is held for the duration of the file copies. For repos with large ignored files this could block concurrent treehouse commands unnecessarily. The post-create hooks already run after the lock is released — seeding feels like it belongs there too, since it's not mutating pool state. Something like:
// after WithStateLock returns, before hooks.Run
if err := git.SeedWorktree(repoRoot, acquired); err != nil {
fmt.Fprintf(os.Stderr, "warning: .worktreeinclude: %v\n", err)
}(Whether to warn or hard-fail is a separate question — see below.)
excludedIncludeSubtree only handles !dir/ negations, not !file
The custom parser skips lines matching !<dir>/ but a bare negation like !.env.local won't be excluded. Given the function comment says "Git does not descend into an excluded directory", I think this is intentional — but it's worth a doc comment clarifying that file-level negations aren't supported so users don't write !secrets/prod.json and wonder why it still gets copied.
Seed errors abort acquire for the missing-file case
If .worktreeinclude lists .env but a contributor doesn't have that file locally (common on a fresh clone), git ls-files returns nothing for it and the seed silently skips it — so this is actually fine in practice. The error path that would bite is an OS-level failure mid-copy. Current behaviour (abort acquire) is defensible, though a warning-and-continue would be more resilient. Worth a deliberate decision either way.
|
Thanks for the review. I moved seeding outside the state lock and changed OS-level copy failures to warn without failing the acquire. I also clarified that Git handles file-level negations directly and added coverage for |
f7b2fe2 to
4f7b2a7
Compare
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for treehouse#48 at |
2bdbbfe to
c650ff6
Compare
|
Rebased onto the latest main and fixed several issues found during review of
|
|
@kunchenguid can this be added to the next release? 🙏 |
|
Speaking as Kun's firstmate: this branch currently has a merge conflict with main ( |
eacd8c8 to
7f6df41
Compare
Confidence Score: 3/5The PR is not yet safe to merge because reused worktrees can retain obsolete seeds and a failed quarantine write can leave a partially seeded worktree available. ResetWorktree preserves ignored files while SeedWorktree only refreshes current selections, so removed or negated seeds survive reuse; additionally, quarantine of a failed reused seed is not durable when WriteState fails, allowing the persisted unleased entry to be selected again. Files Needing Attention: internal/git/git.go and internal/pool/pool.go Reviews (15): Last reviewed commit: "no-mistakes(document): Document .worktre..." | Re-trigger Greptile |
f2cc1b9 to
a1a6e95
Compare
|
Speaking as Kun's firstmate: thanks for the rebase. This is now mergeable (not dirty). I approved fork CI for
|
|
Speaking as Kun's firstmate: newer activity is the no-mistakes raise (Pipeline section is now on the body) plus Greptile still 5/5 on 573e7c6. Opt-in VISION (per rule):
Approved current-head CI 32408269816 and no-mistakes 32408269766. Not merging until those runs are actually green. Helping this PR; not opening a second path for #39. |
2821c7d to
bbe58c4
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
bbe58c4 to
52e6b80
Compare
52e6b80 to
5c6e7c3
Compare
5c6e7c3 to
f5449ea
Compare
|
Speaking as Kun's firstmate: approved current-head CI 32427178130 and Require no-mistakes 32427178081 for VISION (per rule):
|
f5449ea to
c8110ee
Compare
Intent
Add .worktreeinclude support so Treehouse seeds new worktrees with selected ignored or untracked files safely and atomically, preserving file modes and symlinks, rejecting unsafe patterns and paths, remaining cross-platform including Windows, and documenting the feature. Validate the amended current local HEAD. Review or CI findings may be accepted without fixes because the user will inspect them later.
What Changed
.worktreeincludewhenever a worktree is created or reused..worktreeincludepatterns, exclusions, refresh behavior, and failure handling.Risk Assessment
🚨 High: The implementation contradicts two explicit acceptance criteria and contains a reachable tracked-path collision that can make acquisition repeatedly fail and quarantine worktrees.
Testing
Focused seeding and acquisition tests passed, Windows test binaries compiled, and an end-to-end lease acquisition demonstrated selected ignored files being seeded, exclusions remaining absent, executable mode preservation, and safe symlink flattening; the worktree remained unchanged.
Evidence: End-to-end .worktreeinclude CLI transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
internal/git/git.go:280- Intent requires “preserving file modes and symlinks,” but this branch converts every selected symlink into a regular 0666 file containing the target text. For a selectedlinked.env -> ../shared.env, the destination receives a plain text file rather than the symlink. Either preserve safe symlinks while rejecting escaping targets, or confirm that flattening symlinks is the intended requirement.internal/git/git.go:297- Intent requires seeding “atomically,” but each destination is removed and recreated directly, and multiple files are installed sequentially. An observer can therefore see a missing, partial, or mixed old/new seed set; interruption afterRemovecan also leave the old file lost. Stage the complete seed set and commit it through an atomic replacement boundary, or explicitly confirm that quarantine-based containment is acceptable instead of atomic seeding.internal/git/git.go:223- A selected source file is checked only against exact tracked paths and tracked ancestors, not tracked descendants. If the target branch tracksconfig/settings.jsonwhile the source checkout has a selected ignored file namedconfig,isTracked("config")returns false; seeding then tries to replace the trackedconfigdirectory and quarantines every acquired worktree. Include tracked descendants in the collision check so the selected file is skipped.✅ **Test** - passed
✅ No issues found.
go test ./internal/git ./internal/pool -run 'Test(SeedWorktree|Acquire_Seed|Acquire_Quarantine|Acquire_Seeding|Acquire_DoesNotSeed|AcquireDoesNotExecuteSeedFilter|AcquireIgnoresBrokenRequiredSeedFilter)' -count=1Built Treehouse and rantreehouse get --lease --no-fetch --root <temporary-pool>against a temporary Git repository containing.worktreeincludeselections, negation, an executable file, and a symlink; inspected the acquired worktree and saved the transcript.GOOS=windows GOARCH=amd64 go test -c ./internal/git -o <temporary>/git-tests.exeGOOS=windows GOARCH=amd64 go test -c ./internal/pool -o <temporary>/pool-tests.exegit status --shortto confirm testing left the worktree unchanged.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.