Skip to content

feat: Refactor to support generalised directory symlink traversal #852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jun 26, 2025

Conversation

another-rex
Copy link
Collaborator

@another-rex another-rex commented Jun 24, 2025

This PR enables full symlink directory traversal, and sets up the code to make it easier to implement ReadLinkFS in the future.

Here's the list of specific changes:

  • First refactored Node into RootNode and Node.
    • This is fully contained to the first commit, so I highly recommend when reviewing to select the first commit, then select every commit after the first commit, to make it easier to see the diffs.
  • Refactored Node functions to return errors.
  • Replace the existing separated out symlink logic to a generalised one that is done during traversal.
    • This required moving the symlink depth variable onto the pathtree from the chainlayer.
    • Some of this might need to be refactored back out, if we need to implement Readlink, as during symlink resolving, we will need to differentiate the final file being a symlink, vs the directories on the way to the file being symlinks. Since currently we don't exactly need the functionality of the full readlink, I'll leave it as is for now.
    • I have exposed an argument to switch between resolving the final symlink, or returning the symlink as is.
  • Add tests for relative symlinks, and nested relative symlinks.

Copy link
Collaborator

@mleyvajr100 mleyvajr100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for going ahead and refactoring Rex! Appreciate it a lot. Added a few comments, but minor nits + maybe adding more tests.

isWhiteout: false,
mode: filePermission,
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add another test that checks for symlink directories? Unless I missed that somewhere. Ex:

{
	name:    "open file that is symlinked via directory from filled tree",
	chainfs: populatedChainFS,
	path:    "/symlink-dir/foo,
	// "/symlink-dir" resolves to "/dir1", so we should get the virtual file with path "/dir1/foo"
	wantVirtualFile: &virtualFile{
		virtualPath: "/dir1/foo",
		isWhiteout:  false,
		mode:        filePermission,
	},
},

In the setUpChainFS function, we can define another virtual file:

"/symlink-dir": &virtualFile{
	virtualPath: "/symlink-dir",
	isWhiteout:  false,
	mode:        fs.ModeSymlink,
	targetPath:  "/dir1",
},

We could also go another step further and add multiple directory symlinks tests? We could add that on another PR given there's already lots of moving parts here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused the symlink dir to /dir2, and then added some more test cases. PTAL! (fyi if you didn't know, you can filter by changes since last review)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for covering those cases.

@another-rex another-rex requested a review from mleyvajr100 June 25, 2025 23:50
Copy link
Collaborator

@mleyvajr100 mleyvajr100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

isWhiteout: false,
mode: filePermission,
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for covering those cases.

@copybara-service copybara-service bot merged commit b6dd613 into main Jun 26, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants