fix: preserve root path in _strip_protocol so walk('/') works#69
fix: preserve root path in _strip_protocol so walk('/') works#69mixilchenko wants to merge 2 commits into
Conversation
SSHFileSystem inherited the default root_marker of "", so
_strip_protocol("/") collapsed the root to an empty string. walk("/")
then listed the home directory and produced relative paths that were
not considered to exist. Set root_marker = "/" to keep the root, and
add walk tests covering a nested tree and the root case.
There was a problem hiding this comment.
Pull request overview
This PR fixes SSHFileSystem.walk("/") returning incorrect paths by ensuring protocol-stripping preserves the root path. It aligns SSHFileSystem’s root handling with expected absolute-path semantics so directory traversal from / yields paths that exists() recognizes.
Changes:
- Set
SSHFileSystem.root_marker = "/"to prevent_strip_protocol("/")collapsing/into an empty string. - Add
walk()test coverage for a nested directory tree under a test-specific remote path. - Add a regression test ensuring
walk("/")yieldsroot == "/"and absoluteinfo["name"]entries that exist.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_sshfs.py | Adds coverage for walk() on both nested paths and / regression case to ensure returned entries are absolute and valid. |
| sshfs/spec.py | Sets root_marker to preserve / during protocol stripping, fixing root-walk behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@shcheklein I'd like it to be merged and released. What is left? |
shcheklein
left a comment
shcheklein
left a comment
There was a problem hiding this comment.
This PR has (unintentionally) higher blast radius. Probably we need some redesign, at least don't set root_marker. Preserve / only when the caller explicitly supplied an absolute root in strip_protocol or something like this. + we need more tests to ensure that the current behavior is not affected.
SSHFileSysteminherits fsspec's default emptyroot_marker, so_strip_protocol("/")collapsed an explicit root into"". Most SFTP servers (OpenSSH included) reject an empty path with ENOENT for everything exceptREALPATH, sowalk("/")silently yielded nothing andexists("/")/isdir("/")returnedFalse; servers that implement the SFTP draft's "empty path = default directory" rule listed the home directory instead.The fix: after delegating to
super()._strip_protocol(), restore"/"when the caller explicitly passed an absolute root (/,ssh://host/). Empty and relative paths are deliberately unchanged and keep resolving against the home directory.Why not
root_marker = "/"(this PR's first iteration): that would also remap""and baressh://hostfrom the home directory to the root, and shift_parent()results for relative paths — a behavior change for home-relative usage.Behavior change to note in the release:
"/"now refers to the actual root in every operation that goes through fsspec's path stripping —exists,isdir,info,walk,find,glob,du,get,put,rmon"/"act on the root tree instead of failing (OpenSSH) or resolving to$HOME(spec-following servers). Methods that never stripped paths (ls,mkdir,mv, ...) are unaffected.Tests:
walkcoverage over a nested tree, awalk("/")regression test, and_strip_protocolunit tests pinning root/empty/relative/URL behavior.