Repository navigation
refactor(git): centralize linked-worktree validity checks - #580
Conversation
| local path="$1" | ||
|
|
||
| [[ -d "$path" && -f "$path/.git" ]] || return 1 | ||
| git -C "$path" rev-parse --is-inside-work-tree > /dev/null 2>&1 || return 1 |
There was a problem hiding this comment.
Correctness: submodule checkouts misclassified as valid linked worktrees
A Git submodule's checked-out directory also has .git as a regular file (a gitlink, e.g. gitdir: ../.git/modules/<name>), and git -C <submodule_path> rev-parse --is-inside-work-tree also returns true/exit 0 for it, exactly as for a real linked worktree. So base_git_worktree_path_is_valid <submodule_path> returns 0 (success) for a submodule, even though a submodule is not a linked worktree.
Verified empirically:
$ git submodule add ../inner sub # inside a repo
$ cat sub/.git
gitdir: ../.git/modules/sub
$ git -C sub rev-parse --is-inside-work-tree
true
Both guard clauses in the new function pass for sub, so it validates as a linked worktree.
This contradicts the documented contract (README.md / docs/v2-api-contract.md): "Return success only when path is a valid linked Git worktree ... ordinary repositories ... return 1" — a submodule is neither an ordinary repository checkout nor a linked worktree, but isn't excluded by either check. Per #579 / basefoundry/base#2621, this predicate is meant to gate "dirty checks and removal" of worktree-like directories in downstream consumers (Base), so a false positive here is a real correctness gap, not just a doc nuance. Consider adding a submodule-exclusion check, e.g. verifying the .git file's gitdir: target resolves under <repo>/.git/worktrees/ rather than .git/modules/.
There was a problem hiding this comment.
Fixed in commit 857df0d. base_git_worktree_path_is_valid now resolves --git-dir and --git-common-dir and accepts only paths below the repository worktrees/ administrative directory, so submodules under .git/modules/ return status 1. Added a real submodule regression fixture and updated the README, v2 API contract, and changelog. Focused BATS passes 79/79 and the full ./tests/validate.sh gate passes 715/715 plus the repository release and quality contracts.
| rm -f "$historical_manifest" | ||
| error "release reference '$release_ref' does not contain base_api_manifest.yaml" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Cleanup: duplicated historical-manifest-loading boilerplate
The new block here (mktemp + git -C "$repo_root" show "$release_ref:base_api_manifest.yaml" + error handling) is near-verbatim identical to the existing block in check_stable_compatibility() a hundred lines above (same git show, same error message, same cleanup-on-failure pattern; only the mktemp template suffix differs: base-api-release-manifest vs base-api-historical). Consider factoring this into a shared load_historical_manifest() helper that both check_stable_compatibility and check_release_tree call, to avoid maintaining the same git-show/error/cleanup logic in two places.
There was a problem hiding this comment.
Deferred as intentionally non-blocking cleanup for this PR. The duplicated historical-manifest loading paths have identical behavior today, while extracting a helper would widen the diff without changing the release contract. The correctness fix and its regression coverage remain the merge prerequisite; helper extraction can be tracked separately if desired.
…019-refactor-git-centralize-linked-worktree-validity-checks
Fixes #579
Summary
base_git_worktree_path_is_validas the shared linked-worktree validity predicateIssue
Fixes #579; follows the design-level follow-up from basefoundry/base#2621.
Validation
bats lib/bash/git/tests/lib_git.bats./tests/validate.shtests/lint-warnings.shscripts/api-manifest checkgit diff --checkAPI Impact
Adds the stable
base_git_worktree_path_is_valid <path>Git helper. It returns0only for a valid linked worktree,1for ordinary, missing, or stale paths, and2for usage errors. It is read-only and suppresses Git diagnostics for false predicates.Demo Impact: None.
AI context: unchanged; reusable library contract only.\n\n## Review follow-up\n\n- Reject submodule checkouts as linked worktrees by requiring resolved Git metadata under the repository worktrees directory.\n- Add a real submodule regression fixture plus explicit contract documentation.\n- Defer the non-blocking historical-manifest helper extraction to a separate cleanup change.