Repository navigation
Conversation
…sition-resolved common Git directory through Git and jj seeding, including colocated jj, while preserving non-colocated discovery. Added executable regressions for older-Git fallback, single config lookup, and dangling/absent sidecar behavior; restored the authorized state comment. Verified make test, make lint, Windows cross-build, focused tests, and real jj tests. External PR Testing-section publication was not performed in this ci phase
Owner
Author
|
Superseded by the upstream PR kunchenguid#191 (same change on current upstream main). The local build is made from branch fm/th-189-upstream, which stays. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
go ahead, build and test on fork then as upstream PR
Build the change proposed in upstream issue kunchenguid#189 as corrected in kunchenguid#189 (comment): layer the include manifest the way Git layers ignore rules. Decisions taken: (1a) literal Git precedence, lowest to highest: user layer, then clone layer, then the repository layer, so the committed file wins; (2) the user layer is a treehouse-defined Git config key
worktree.includeFile, resolved with normal Git config lookup (--pathtyping, includes/includeIf) in the main source checkout, with no implicit XDG fallback; (3) the clone layer is$GIT_COMMON_DIR/info/worktreeincludeof the source clone, and creating that file is the opt-in; (4)--include-filekeeps replacing every layer, an empty file still meaning no seeds, and lower-layer read errors do not apply when it is given; (5) layers are combined as one ordered pattern stream fed to Git, preserving file boundaries and stripping a leading UTF-8 BOM per file, so!negation works across layers; (6) persistent-layer read errors surface at seeding time with today's remove-or-quarantine handling; (7) the same behavior for both jj modes. Selection stays a separate predicate intersected with--exclude-standard; an explicitly configured but missing or unreadable file is an error, an absent key or sidecar means no rules, an empty configured value is a configuration error. Copy, inventory, cleanup and pool state are untouched.The referenced design report's implementation and required regression substance:
Smallest coherent implementation
Limit production work to the shared seeding resolver/matcher and contract text. In selectedSeedPathsEnv, preserve the non-nil override short circuit. Only the nil/default path discovers and reads the configured user file, common-directory sidecar, and destination committed blob. An empty/missing committed file must no longer terminate the search early. Resolve metadata/config using source repoRoot plus existing gitEnv; keep committed revision lookup exactly as now. Read all chosen layers before matching/copying. Join them in the selected precedence order with file-boundary normalization; keep the two-stage selection/ignore intersection and copy implementation untouched. Keep the existing pool and backend override parameter, state schema, HMAC, and cleanup untouched. Both backends already enter this shared code. Update comments so nil means the layered default, not committed-only. Update README seeding text/flag table, get help, and the design seeding note: precise order, locations, opt-in/trust statement, root-relative pattern semantics, directory-negation caveat, errors, and all-layer override. No new config subsystem.
Read new default sources at seeding time, after a slot was created/reset. Existing failure paths remove fresh slots or quarantine reused ones. Do not promise that a bad persistent file leaves the pool untouched. The explicit flag's existing pre-acquisition read/error guarantee stays unchanged.
Required regression work
Extend existing test files, using isolated HOME/XDG/Git/JJ config and deterministic filesystem/process events:
Before shipping: make test, make lint, GOOS=windows go build ./..., real CLI leased/path/JSON and interactive smoke tests for create/reuse/return, and real jj tests with jj installed. Capture an unconfigured before/after path/content comparison and a negative input that must fail.
What Changed
worktree.includeFile, the source clone's common-directory sidecar, and the destination revision's committed manifest, preserving Git pattern precedence and file-boundary handling.--include-fileas a complete replacement that bypasses default-layer discovery and errors, while surfacing persistent-layer read failures through existing acquisition handling.Risk Assessment
Testing
Validation on pushed head
db51f32418b62a3dac1a96ac9a13d650e7909132completed with all six fork CI checks green, including Linux, macOS, and Windows tests. Local full-suite, lint, Windows cross-build, focused seeding/lifecycle regressions, and the supplemental live checks below passed. The initial pipeline analyzer was inconclusive (4/6 live scenarios); Test approval records an authorized exception supported by completing the missing scenarios, not a clean initial analyzer verdict.Supplemental live pattern-boundary validation used the real built Treehouse CLI with a UTF-8 BOM in each layer, CRLF, missing final newlines, and cross-layer
!negation. It copied exactlyalpha.env,clone.env, andrepo.env, with byte-for-byte source parity. The negatedveto.env, unselected ignored file, and an explicitly selected but unignoredclear.txtwere absent.jj was initially unavailable locally. An official, temporary, worktree-only darwin-arm64 jj 0.46.0 binary was then used; its release archive's published SHA-256 was verified:
43daa37ce0f9716cdf6fb65a29cbcae74a3160aef048cf3cb3d166d41cb4ab58. The full real jj package tests ran without skips, including both colocated and non-colocated layer/revision/store-lookup regressions; Git/jj CLI override tests also passed. Real Treehouse CLI scenarios in both jj modes verified layered copies, an empty jj working-copy commit after snapshot, and authenticated return removing inventoried seeds while preserving an unmanaged ignored file. These supplemental scenarios and real jj package tests were repeated on the pushed head above.The fork CI workflow does not install jj and its Go output does not prove real jj execution, so CI package passes are not claimed as jj parity evidence; the verified local runs provide that evidence. The unchanged analyzer table and evidence below (and the Pipeline Test block) are the historical initial record, superseded for the two missing scenarios by the supplemental live validation above.
go test ./cmd -run 'TestGetIncludeLayersAndOverride|TestGetIncludeFileReplacesCommittedManifest' -count=1go test ./internal/pool -run 'TestAcquire_(PersonalLayersRefreshAndInventoryCleanup|PersistentLayerReadErrorsRemoveOrQuarantine)' -count=1go test ./cmd -run 'TestGetIncludeLayersAndOverride|TestGetIncludeFileReplacesCommittedManifest' -count=1jjis not installed (command -v jjfailed); the targeted tests explicitly skipped withjj is not installed. No workspace-local jj executable/source was provide…Evidence: Live layered CLI smoke
Evidence: Live lifecycle and override smoke
Evidence: Live unconfigured baseline smoke
Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
internal/vcs/gitvcs/gitvcs.go:570-defaultWorktreeIncludeunconditionally invokesgit rev-parse --path-format=absolute --git-common-dirat line 570. On Git versions that do not support--path-format, every default acquisition fails before checking the committed manifest—even when the user key, sidecar, and committed file are all absent—regressing the required no-layer behavior. The existingCommonGitDirhelper at lines 78-89 already preserves compatibility with a fallback; use the equivalent fallback while retaining the supplied Git environment.internal/vcs/gitvcs/gitvcs.go:580- The clone sidecar is treated as absent wheneveros.ReadFilereturnsfs.ErrNotExistat line 580. A dangling symlink at$GIT_COMMON_DIR/info/worktreeincludeis a present, opted-in persistent layer but producesENOENTwhile reading its target, so the code silently skips it and may seed only lower-precedence committed rules instead of surfacing the required persistent-layer read error. Distinguish a missing sidecar entry from a read failure (for example withLstat) before applying the absent-sidecar exception.🔧 Fix applied.
✅ Re-checked - no issues remain.
go test ./cmd -run 'TestGetIncludeLayersAndOverride|TestGetIncludeFileReplacesCommittedManifest' -count=1go test ./internal/pool -run 'TestAcquire_(PersonalLayersRefreshAndInventoryCleanup|PersistentLayerReadErrorsRemoveOrQuarantine)' -count=1go test ./cmd -run 'TestGetIncludeLayersAndOverride|TestGetIncludeFileReplacesCommittedManifest' -count=1jjis not installed (command -v jjfailed); the targeted tests explicitly skipped withjj is not installed. No workspace-local jj executable/source was provide…go test ./internal/vcs/gitvcs -run 'TestSeed' -count=1go test ./internal/pool -run 'TestAcquire_(PersonalLayersRefreshAndInventoryCleanup|PersistentLayerReadErrorsRemoveOrQuarantine|ManifestOverrideRefreshesAndRemovesSeedsOnReuse)' -count=1go test ./cmd -run 'TestGetInclude(LayersAndOverride|FileReplacesCommittedManifest|FileReadFailureLeavesPoolUntouched)' -count=1go test ./internal/vcs/jjvcs -run 'TestSeed' -count=1(skipped: jj unavailable)Built and drove the target CLI against isolated disposable Git repositories; captured evidence in the three listed logs.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.