fix(git): fix canonical_root for worktrees and subdirectory projects#672
fix(git): fix canonical_root for worktrees and subdirectory projects#672anivaryam wants to merge 4 commits into
Conversation
git rev-parse --git-common-dir outputs a path relative to the directory passed via -C (the indexed path), not to worktree_root. When the indexed path is a subdirectory of the repo root, or a linked worktree sibling, joining the relative git_common_dir with worktree_root produced a path with unresolved ".." components. Stripping the "/.git" suffix then left e.g. "/workspace/.." rather than "/workspace". Two changes in derive_canonical_root(): 1. Use input_path (the original indexed directory) as the join base for relative git_common_dir paths, so "../.git" from scripts/ resolves against scripts/, not against the already-resolved worktree root. 2. Call realpath() (POSIX) / _fullpath() (Windows) on the joined .git path before stripping the suffix, collapsing any ".." components into the canonical filesystem path. The .git directory always exists for a valid repository, so realpath() succeeds. Add test_git_context.c covering all three cases: - repo indexed from its root (existing behaviour, must not regress) - project inside a repo, indexed from a subdirectory (issue DeusData#659) - project in a linked git worktree (issue DeusData#659 primary case) Fixes DeusData#659 Signed-off-by: anivaryam <anivaryam.dev@gmail.com>
check-no-test-skips.sh prohibits plain SKIP(); git worktree add is available since git 2.5 (2015) so failure is a real error, not a platform/version skip. Signed-off-by: anivaryam <anivaryam.dev@gmail.com>
- Replace MAX_PATH (requires windows.h) with 4096 in both src/git/git_context.c and tests/test_git_context.c; use sizeof(resolved) instead of the literal for _fullpath() - Wrap canonical_root_linked_worktree body in #ifdef _WIN32 / #else / #endif so realpath() is excluded from Windows compilation (SKIP_PLATFORM is a runtime skip — the body still compiled) Signed-off-by: anivaryam <anivaryam.dev@gmail.com>
|
Huge thanks for opening this PR and for the work you put into it. The maintainer shop is currently full, so this may sit for a bit before it gets a proper review. We will come back to this as soon as possible with real feedback; I wanted to make sure it did not sit unacknowledged in the meantime. |
|
Thanks, this looks focused on #659. Before merge, please add DCO sign-off to the non-merge commit. Also, because the regression test shells out to |
|
Thank you @anivaryam — this is a correct and well-diagnosed fix. You nailed the root cause: I've taken your Closing this as superseded by the distilled PR, with full credit to you. Really appreciate the careful work — the fix ships in the next release. 🙏 |
detect_changes returned an empty impacted_symbols set for projects indexed inside a git worktree or from a repo subdirectory (DeusData#659): canonical_root was computed wrong. `git rev-parse --git-common-dir` emits a path relative to the directory passed via -C (input_path), NOT to worktree_root, and the "/.git" suffix was stripped textually without resolving ".." components. So e.g. "../.git" joined against worktree_root, or "/ws/scripts/../.git" strip, left an unresolved ".." and never matched the project's stored root path. Fix (src/git/git_context.c): join the relative --git-common-dir against input_path (the -C dir) instead of worktree_root, and realpath()/_fullpath()- normalize the result BEFORE stripping "/.git" (the .git dir always exists for a valid repo, so resolution succeeds). This is consistent with the existing realpath'd root_path used for prefix matching (mcp.c). Reproduce-first: tests/test_git_context.c. The subdirectory case is the genuine guard — a repo indexed from a subdir yields a *relative* --git-common-dir, so the unfixed code leaves an un-normalized "<root>/subdir/.." (verified RED; GREEN after the realpath fix). The linked-worktree case is a supporting invariant, not the DeusData#659 reproducer: git that emits an *absolute* worktree --git-common-dir (e.g. 2.48.x) doesn't manifest the bug, so it passes either way; it still enforces the worktree->main-root invariant. All three git-based tests SKIP_PLATFORM on Windows (the CI shell there cannot init a repo via system()). Distilled from DeusData#672 (thanks @anivaryam); that PR's production fix is taken verbatim. Its repo_root/subdir tests ran git unconditionally and failed the Windows CI leg ("failed to init git repo"); here all three git-based tests are Windows-skipped so ci-ok is green cross-platform. Supersedes DeusData#696, which only realpath'd after the strip without changing the join base and so still yielded the workspace parent for the subdirectory case. Closes DeusData#659 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com> Co-Authored-By: anivaryam <anivaryam.dev@gmail.com>
Summary
Fixes #659 —
detect_changesreturns emptyimpacted_symbolsfor projects indexed inside git worktrees or as repo subdirectories becausecanonical_rootwas computed incorrectly.Root cause:
git rev-parse --git-common-diroutputs a path relative to the directory passed via-C(the indexed path,input_path), not toworktree_root. Two errors inderive_canonical_root():git_common_dir(e.g."../.git") was joined withworktree_rootinstead ofinput_path. For a project atworkspace/scripts/,worktree_rootisworkspace/but the relative path is relative toscripts/— joining it withworkspace/producedworkspace/../.git, which strips toworkspace/...scripts/../.git), stripping the/.gitsuffix leavesscripts/..— a path with unresolved..components. Without normalization this is stored as-is and never matches the project's stored root path.Fix (
src/git/git_context.c):input_path(the original indexed directory) as the join base for relativegit_common_dirpaths.realpath()(POSIX) /_fullpath()(Windows) on the joined.gitpath before stripping the suffix. The.gitdirectory always exists for a valid repository, so the call succeeds and collapses any..components.Tests (
tests/test_git_context.c):Three new tests in
suite_git_context:canonical_root_repo_root— normal repo indexed from its root (regression guard)canonical_root_subdir— project indexed from a subdirectory of the repo (issue detect_changes: impacted_symbols always empty for projects inside git worktrees (canonical_root miscalculated) #659)canonical_root_linked_worktree— project in a linked git worktree (issue detect_changes: impacted_symbols always empty for projects inside git worktrees (canonical_root miscalculated) #659 primary case)Test plan
canonical_root_repo_rootpassescanonical_root_subdirpasses (was failing before fix)canonical_root_linked_worktreepasses (was failing before fix)make testsuite has no regressions