fix: make GitWalkerFs#content handle symlinks correctly#2271
Merged
jcubic merged 5 commits intoisomorphic-git:mainfrom Jan 23, 2026
Merged
fix: make GitWalkerFs#content handle symlinks correctly#2271jcubic merged 5 commits intoisomorphic-git:mainfrom
GitWalkerFs#content handle symlinks correctly#2271jcubic merged 5 commits intoisomorphic-git:mainfrom
Conversation
📝 WalkthroughWalkthroughGitWalkerFs now returns symlink targets (via readlink) as entry content; tests and fixture-copy helpers were updated to preserve and validate symlink behavior. A new contributor, Mateusz Burzyński (Andarist), was added to contributors metadata and README. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Walker as GitWalkerFs
participant FS as Filesystem (OS)
participant Cache as Cache
participant Git as Git object store
Walker->>Cache: lookup(entry.path)
alt cached
Cache-->>Walker: cached entry
else not cached
Walker->>FS: stat(entry.path)
FS-->>Walker: mode
alt mode is symlink (0o120000)
Walker->>FS: readlink(entry.path)
FS-->>Walker: targetPath
Walker->>Git: hashBlob(targetPath) %% (hash blob of targetPath string as content)
Git-->>Walker: blobOid
Walker->>Cache: store(entry with content=targetPath, oid=blobOid, mode=symlink)
Walker-->>Caller: entry(content=targetPath, oid=blobOid, mode=symlink)
else regular file
Walker->>FS: readFile(entry.path)
FS-->>Walker: fileContent
Walker->>Walker: apply core.autocrlf if configured
Walker->>Git: hashBlob(fileContent)
Git-->>Walker: blobOid
Walker->>Cache: store(entry with content=fileContent, oid=blobOid, mode=file)
Walker-->>Caller: entry(content=fileContent, oid=blobOid, mode=file)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
Andarist
commented
Jan 19, 2026
| expect(entry.content).toBe('a.txt') | ||
| }) | ||
|
|
||
| it('symlink oid works for non-existent targets', async () => { |
Contributor
Author
There was a problem hiding this comment.
I ran into a crash caused by null content here:
isomorphic-git/src/models/GitWalkerFs.js
Line 108 in 93d3ef9
The content method was called by oid method and the stack trace looked like this:
TypeError: Cannot read properties of null (reading 'length')
at GitWalkerFs.content (/ghcommit/node_modules/.pnpm/[email protected]/node_modules/isomorphic-git/index.cjs:4249:37)
at /ghcommit/node_modules/.pnpm/[email protected]/node_modules/isomorphic-git/index.cjs:4276:27
at /ghcommit/node_modules/.pnpm/[email protected]/node_modules/isomorphic-git/index.cjs:1026:16
Contributor
|
If |
Contributor
Author
|
@sdarwin thank you for a promp review, I pushed out the requested change |
Member
|
There are broken tests: |
Contributor
Author
|
@jcubic initially I ran into some classic jest/esm/node/whatever issues when trying to run tests locally so I couldn't quickly verify everything. I solved this and was able to investigate and fix those failures. It turns out that:
|
Member
|
Can you explain why you don't read the target of the symlink? What is the use of the walk function if it doesn't return the content of the file, only a path? If you run the walk on the code base, you will need to check if the content is path or file content. Isn't it supposed to be something like: let filepath = `${dir}/${entry._fullpath}`;
if ((await entry.mode()) >> 12 === 0b1010) {
filepath = await fs.readlink(filepath);
}
const config = await this._getGitConfig(fs, gitdir)
const autocrlf = await config.get('core.autocrlf')
const content = await fs.read(filepath, { autocrlf }) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@__tests__/test-walk-in-submodule.js`:
- Around line 592-649: Replace the hardcoded symlink mode 0o120000 in both
tests' assertions (the expect(entry.mode).toBe(0o120000) lines) with the
BrowserFS-aware mode value computed earlier in this file (the same
helper/variable used above to adapt for BrowserFS’ 555/exec-bit quirk), so both
symlink-mode assertions use that shared expected-mode variable instead of the
literal 0o120000.
Contributor
Author
It's about:
From what I understand, I added new tests to demo what I mean. |
Member
|
ok, make sense. |
Member
|
🎉 This PR is included in version 1.36.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Feb 4, 2026
Closed
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
4 participants
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.
I'm fixing a bug or typo
npm run add-contributorand follow the prompts to add yourself to the READMESummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.