fix: support git worktrees in discoverGitdir (absolute gitdir paths)#2293
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughdiscoverGitdir now distinguishes absolute vs. relative gitdir paths read from a .git file and returns either the absolute path directly or resolves relative paths against the .git file location. New tests cover directory .git, submodule-style relative gitdir, and worktree-style absolute gitdir. discoverGitdir is re-exported via internal-apis. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
| function isAbsolute(filepath) { | ||
| return ( | ||
| filepath.startsWith('/') || /^[a-zA-Z]:[\\/]/.test(filepath) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
I wasn't sure whether this would run only in node or also in browser? I sadly don't know enough about the project :/
If this is only run in node I would switch it to use node's path.isAbsolute()
|
Failed tests: |
2a02798 to
e584722
Compare
|
@jcubic I've opened #2294 and based this branch on top of that PR. Will rebase once that one is merged. But I'm not sure if maybe it's a policy of the project to not import from |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/test-discoverGitdir-worktree.js (1)
73-73: Consider adding a Windows absolute path test case.The hardcoded Unix-style absolute path
/some/absolute/path/...validates Unix worktrees. For completeness, consider adding a test with a Windows-style path likeC:\\repo\\.git\\worktrees\\branchto ensure theisAbsoluteregex handles both formats correctly.💡 Example additional test case
it('returns Windows absolute gitdir path directly', async () => { const fsp = new FileSystem(_fs) const worktreeDir = path.join(tmpDir, 'win-worktree') await _fs.promises.mkdir(worktreeDir) const dotgit = path.join(worktreeDir, '.git') const windowsAbsoluteGitdir = 'C:\\repo\\.git\\worktrees\\my-branch' await _fs.promises.writeFile(dotgit, `gitdir: ${windowsAbsoluteGitdir}\n`) const result = await discoverGitdir({ fsp, dotgit }) expect(result).toEqual(windowsAbsoluteGitdir) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/test-discoverGitdir-worktree.js` at line 73, Add a Windows-style absolute path test to __tests__/test-discoverGitdir-worktree.js that mirrors the existing Unix test: create a worktree dir, write a .git file containing a Windows absolute gitdir string (e.g. windowsAbsoluteGitdir = 'C:\\repo\\.git\\worktrees\\my-branch'), call discoverGitdir({ fsp, dotgit }) and assert the result equals windowsAbsoluteGitdir; this ensures the isAbsolute detection/regex in discoverGitdir handles Windows paths as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/test-discoverGitdir-worktree.js`:
- Line 73: Add a Windows-style absolute path test to
__tests__/test-discoverGitdir-worktree.js that mirrors the existing Unix test:
create a worktree dir, write a .git file containing a Windows absolute gitdir
string (e.g. windowsAbsoluteGitdir = 'C:\\repo\\.git\\worktrees\\my-branch'),
call discoverGitdir({ fsp, dotgit }) and assert the result equals
windowsAbsoluteGitdir; this ensures the isAbsolute detection/regex in
discoverGitdir handles Windows paths as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c7b6704-bc6d-4ca7-b632-4079f65503ff
📒 Files selected for processing (6)
.all-contributorsrcREADME.md__tests__/test-BaseError.js__tests__/test-discoverGitdir-worktree.jssrc/errors/BaseError.jssrc/utils/discoverGitdir.js
Just check what the other parts of the tests are doing. |
e584722 to
a7712e5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/test-discoverGitdir-worktree.js (1)
65-77: Consider adding Windows-style absolute path coverage.This test validates Unix-style absolute paths (
/some/absolute/...), but theisAbsolutehelper indiscoverGitdir.jsalso handles Windows drive letters (C:\...). Adding a test case for Windows-style paths would improve confidence in cross-platform behavior.💡 Optional: Add Windows-style absolute path test
+ it('handles Windows-style absolute gitdir path', async () => { + const fsp = new FileSystem(_fs) + const worktreeDir = path.join(tmpDir, 'win-worktree') + await _fs.promises.mkdir(worktreeDir) + const dotgit = path.join(worktreeDir, '.git') + + const windowsGitdir = 'C:\\Users\\dev\\repo\\.git\\worktrees\\feature' + await _fs.promises.writeFile(dotgit, `gitdir: ${windowsGitdir}\n`) + + const result = await discoverGitdir({ fsp, dotgit }) + expect(result).toEqual(windowsGitdir) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/test-discoverGitdir-worktree.js` around lines 65 - 77, Add a Windows-style absolute path case to the existing test for discoverGitdir: create a test similar to "does not corrupt absolute paths by joining with dirname" that writes a .git file containing a Windows drive-letter absolute path (e.g. "C:\\some\\absolute\\path\\.git\\worktrees\\my-branch"), call discoverGitdir({ fsp, dotgit }) and assert the returned result equals that Windows path and does not contain the worktreeDir; this verifies discoverGitdir and its isAbsolute handling for Windows paths (remember to escape backslashes in the test string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/test-discoverGitdir-worktree.js`:
- Around line 65-77: Add a Windows-style absolute path case to the existing test
for discoverGitdir: create a test similar to "does not corrupt absolute paths by
joining with dirname" that writes a .git file containing a Windows drive-letter
absolute path (e.g. "C:\\some\\absolute\\path\\.git\\worktrees\\my-branch"),
call discoverGitdir({ fsp, dotgit }) and assert the returned result equals that
Windows path and does not contain the worktreeDir; this verifies discoverGitdir
and its isAbsolute handling for Windows paths (remember to escape backslashes in
the test string).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f76405d7-a9b3-4501-a46c-af55b5311ebd
📒 Files selected for processing (3)
__tests__/test-discoverGitdir-worktree.jssrc/internal-apis.jssrc/utils/discoverGitdir.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/discoverGitdir.js
Ahh so if I understand correctly we would not import from Let's wait for the pipelines to run through tho, locally tests executed fine but let's see👀 |
|
Can you test the public API on a real Git repository? Something that breaks before your change, and after your PR it works. Right now I'm not sure exactly what you're testing. You've changed internals and confirmed that it works like you assume it should work. |
922b759 to
2378f7f
Compare
Hey, I added a test that uses the public api the exact way I came across the bug, which is with the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/test-worktree.js`:
- Line 4: The import statement currently imports an unused symbol "add" from
'isomorphic-git' alongside "statusMatrix"; remove "add" from the import so only
"statusMatrix" is imported (verify there are no usages of add in the test file
and update the import in the top-level import line that references statusMatrix
and add accordingly) to silence the no-unused-vars lint warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26f83c81-869b-4403-bd91-ea7db2db116a
📒 Files selected for processing (1)
__tests__/test-worktree.js
|
🎉 This PR is included in version 1.37.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation
Fixes #735 — "Unresolvable reference when .git is a file (git worktree support)"
Changes
src/utils/discoverGitdir.js: AddisAbsolute()check beforejoin(). If the gitdir pointer is absolute (Unix/or Windows drive letterC:\), return it directly. Relative paths (submodules) continue through the existingjoin(dirname(dotgit), submoduleGitdir)path unchanged.__tests__/__helpers__/FixtureFSWorktree.js: New test helper (followingFixtureFSSubmodule.jspattern) that creates a worktree-style fixture with a.gitfile pointing to an absolute gitdir path.__tests__/test-worktree.js: Integration tests verifyingcurrentBranchandresolveRefwork from a simulated worktree.__tests__/test-discoverGitdir-worktree.js: Unit tests fordiscoverGitdircovering directory, relative path (submodule), and absolute path (worktree) cases.Note: This PR is AI generated and not hand written. I had a look at the contributor docs and reviewed it myself. However note that I still don't have a lot of knowledge about this project, so I also understand if you don't have capacity to review this :)
Summary by CodeRabbit
New Features
Bug Fixes
Tests