Fix worktree detection for worktrees inside .git directory #154
Conversation
Previously, worktrees located as children of the git root (e.g., in .git/workspaces/) were not properly detected, causing commands like GinStatus to show the git root's status instead of the worktree's status. This fix adds upward directory traversal to find worktree roots and improves git rev-parse execution to handle worktrees in .git directories. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
""" WalkthroughThe changes add a function to detect the filesystem root and modify the Git worktree finder to traverse directories upward to find a worktree root. The Changes
Error: Could not generate a valid Mermaid diagram after multiple attempts.
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
denops/gin/git/finder.ts (2)
6-6: Fix misleading comment.The comment says "seconds" but the value
30000represents milliseconds (30 seconds).-const ttl = 30000; // seconds +const ttl = 30000; // milliseconds
76-89: Consider more specific error handling.The function correctly identifies worktree roots, but the blanket catch block might hide real errors. Consider handling specific error types differently.
async function isWorktreeRoot(currentPath: string): Promise<boolean> { try { const gitPath = resolve(currentPath, ".git"); const stat = await Deno.stat(gitPath); if (stat.isFile) { // Found a .git file, verify it's a valid git worktree await revParse(currentPath, ["--git-dir"]); return true; } - } catch { - // Either .git doesn't exist or it's not a valid worktree + } catch (e) { + // Only ignore expected errors (file not found, not a worktree) + if (!(e instanceof Deno.errors.NotFound)) { + // Log unexpected errors for debugging + console.debug("Unexpected error in isWorktreeRoot:", e); + } } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
denops/gin/git/finder.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
denops/gin/git/finder.ts (2)
denops/gin/git/process.ts (1)
execute(42-56)denops/gin/util/text.ts (1)
decodeUtf8(14-16)
🔇 Additional comments (2)
denops/gin/git/finder.ts (2)
21-34: Well-implemented upward directory traversal.The logic correctly searches upward from the current directory to find a worktree root, with proper caching when a worktree is found. The dual condition for detecting the root directory ensures compatibility across different platforms.
92-109: Excellent implementation of retry logic for.gitdirectories.The function correctly handles the edge case of running
git rev-parseinside.gitdirectories by implementing a smart retry mechanism. The path manipulation usingSEPARATORensures cross-platform compatibility.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 58.91% 60.61% +1.70%
==========================================
Files 17 17
Lines 881 904 +23
Branches 108 118 +10
==========================================
+ Hits 519 548 +29
+ Misses 362 356 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
denops/gin/git/finder.ts
Outdated
| // Search upward from current directory to find worktree root | ||
| let currentPath = path; | ||
| while ( | ||
| currentPath && currentPath !== "/" && |
There was a problem hiding this comment.
Sorry, I forgot about Windows.
Fixed in 0a99e14 and also added tests for worktrees inside .git.
denops/gin/git/finder.ts
Outdated
| // If it fails and we're in a .git directory, try from parent | ||
| const terms = cwd.split(SEPARATOR); | ||
| if (terms.includes(".git")) { | ||
| const index = terms.indexOf(".git"); | ||
| const parentCwd = terms.slice(0, index).join(SEPARATOR); | ||
| const stdout = await execute(["rev-parse", ...args], { cwd: parentCwd }); | ||
| const output = decodeUtf8(stdout); | ||
| return resolve(output.split(/\n/, 2)[0]); | ||
| } |
There was a problem hiding this comment.
Why do we need this fallback?
There was a problem hiding this comment.
I kept the fallback for compatibility reasons, but on second thought, it wasn't needed since we're running the command in the intended directory from the beginning. Just my mistake for not catching that.
Removed it in d93a18d
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🛑 Comments failed to post (2)
denops/gin/git/finder.ts (1)
32-42: 💡 Verification agent
🧩 Analysis chain
Excellent upward traversal logic for worktree detection.
The implementation correctly addresses the PR objective by searching upward from the current directory to find worktree roots. The early return with caching when a worktree is found and the fallback to regular repository detection is well-designed.
However, verify the test coverage for the worktree detection path:
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for the new worktree detection logic # Search for tests that create worktrees inside .git directories rg -A 10 -B 5 "worktree.*\.git" --type ts # Look for tests specifically covering the upward traversal logic ast-grep --pattern $'while (!isFilesystemRoot($$$)) { $$$ }'Length of output: 595
Add test coverage for upward worktree detection
We didn’t find any existing tests that exercise thewhile (!isFilesystemRoot(currentPath)) { … }block or simulate a nested worktree under.git/worktrees. Please add tests covering:
- A directory hierarchy with a
.git/worktrees/<name>folder to ensure early return and caching- The case where no worktree is found (traverse to filesystem root and fallback)
- Cache behavior when the same path is queried multiple times
Suggested locations:
- Production code:
denops/gin/git/finder.ts(lines 32–42)- New test file, e.g.
tests/git/finder.test.ts🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-39: denops/gin/git/finder.ts#L36-L39
Added lines #L36 - L39 were not covered by tests🤖 Prompt for AI Agents
In denops/gin/git/finder.ts around lines 32 to 42, add new test cases in a test file like tests/git/finder.test.ts to cover the upward traversal worktree detection logic. Create tests that simulate a directory hierarchy containing a .git/worktrees/<name> folder to verify early return and caching behavior, tests where no worktree is found to confirm traversal to filesystem root and fallback, and tests that query the same path multiple times to validate cache usage. This will ensure the while loop and caching logic are properly exercised and verified.denops/gin/git/finder_test.ts (1)
8-60: 🛠️ Refactor suggestion
Comprehensive and well-structured test for filesystem root detection.
The test effectively covers:
- Cross-platform root paths (Windows and Unix)
- Upward traversal behavior
- Safety limits to prevent infinite loops
- Edge cases with root directory validation
However, the main PR objective (worktree detection inside
.gitdirectories) lacks test coverage. Consider adding a test case that reproduces the specific bug scenario:Deno.test({ name: "findWorktree() correctly detects worktrees inside .git directory", fn: async () => { await using sbox = await prepare(); // Create a worktree inside the .git directory const worktreePath = join(sbox.path, ".git", "worktrees", "test-branch"); await $`git worktree add ${worktreePath} -b test-branch`; // Change to the worktree directory Deno.chdir(worktreePath); // Should detect the worktree root, not the main repository assertEquals(await findWorktree("."), worktreePath); }, sanitizeResources: false, sanitizeOps: false, });This would directly test the bug fix described in the PR objectives.
🤖 Prompt for AI Agents
In denops/gin/git/finder_test.ts around lines 8 to 60, while the existing tests thoroughly check filesystem root detection, there is no test covering the main PR objective of detecting worktrees inside .git directories. Add a new Deno.test that sets up a git worktree inside a .git/worktrees subdirectory, switches to that worktree directory, and asserts that findWorktree correctly identifies the worktree root path. This test should create the worktree using git commands, change the current directory to the worktree, and verify the detection, ensuring coverage of the specific bug fix.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
denops/gin/git/finder.ts(5 hunks)denops/gin/git/finder_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- denops/gin/git/finder_test.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
denops/gin/git/finder.ts
[warning] 36-39: denops/gin/git/finder.ts#L36-L39
Added lines #L36 - L39 were not covered by tests
[warning] 89-92: denops/gin/git/finder.ts#L89-L92
Added lines #L89 - L92 were not covered by tests
🔇 Additional comments (4)
denops/gin/git/finder.ts (4)
1-1: LGTM: Unnecessary import removed.The removal of the
SEPARATORimport is appropriate since it's no longer used after the refactoring.
10-19: Well-designed cross-platform filesystem root detection.The
isFilesystemRootfunction provides a clean, cross-platform solution for detecting the filesystem root. The approach of comparing a path with its parent usingresolve(currentPath, "..")correctly handles Windows, Linux, and macOS paths, addressing the previous concern about Windows compatibility.
49-49: Improved error handling with explicit type casting.The explicit casting to
Errortype before caching provides better type safety and clarity about the expected error handling behavior.Also applies to: 73-73
99-103: Simplified revParse function improves maintainability.The removal of the previous fallback logic that adjusted the working directory makes the function more straightforward and easier to understand. The worktree-specific logic is now properly handled in the upward traversal within
findWorktree, which is a better separation of concerns.
Issue Overview
Commands like
:GinStatussometimes show the status of the root repository instead of the correct worktree, even when a worktree is in use. When the worktree is created under the Git root directory (i.e., under.git/worktrees/), the plugin fails to detect it properly and ends up referencing the root repository's state.Reproduction Patterns
Correct Pattern
Git root:
Worktree:
If the worktree is created at the same level as (or in a directory separate from) the Git root, opening a file in
workspace1and running:GinStatuscorrectly shows the status of the worktree at<any_path>/workspace1.Bug Pattern
Git root:
Worktree (created under the Git root):
In this case, opening a file in
workspace1and running:GinStatusdisplays the status of the Git root (<any_path>/example) instead of the worktree, even though the worktree is located at<any_path>/example/.git/worktrees/workspace1.Expected Behavior
Even when a worktree is created under the Git root directory (under
.git/worktrees/), the plugin should correctly identify the root directory of the worktree to which the opened file belongs (for example,<any_path>/example/.git/worktrees/workspace1) and display that worktree's status.Solution
This PR fixes the worktree detection logic by:
Adding upward directory traversal: The plugin now searches upward from the current directory to find worktree roots by checking for
.gitfiles (which indicate a worktree).Implementing proper worktree validation: A new
isWorktreeRootfunction validates that a directory containing a.gitfile is indeed a valid git worktree.Improving
git rev-parseexecution: TherevParsefunction now first attempts to execute from the current directory, which correctly handles worktrees inside.gitdirectories. It only falls back to parent directory handling if the initial attempt fails.These changes ensure that worktrees located within the git root directory structure are properly detected and their status is correctly displayed.
Summary by CodeRabbit
Note
This motivated me to want to manage the worktree in .git.
https://x.com/mizchi/status/1926974534541844532