Repository navigation
Conversation
mountpoint determines whether a path is a mount point by comparing it with its parent. The implementation was checking the process working directory's parent instead, which could make regular directories look like mount points. Use the target path parent for the comparison and return util-linux's non-mountpoint exit status so this behavior is covered by a focused regression test. Signed-off-by: favilances <favilances@proton.me>
|
this one has been conflicting for a while, could you please rebase it? otherwise, i will close it, thanks |
|
|
||
| if let Some(path) = path { | ||
| if is_mountpoint(path) { | ||
| if is_mountpoint(path)? { |
There was a problem hiding this comment.
the error will lack the path, util-linux prints mountpoint: /foo: No such file or directory.
please use map_err_context
| println!("{path} is a mountpoint"); | ||
| } else { | ||
| println!("{path} is not a mountpoint"); | ||
| process::exit(32); |
There was a problem hiding this comment.
please don't add process::exit, use uucore::error::set_exit_code(32) and return Ok
|
|
||
| let dev = metadata.dev(); | ||
| let inode = metadata.ino(); | ||
| let parent = path.parent().filter(|p| !p.as_os_str().is_empty()); |
There was a problem hiding this comment.
for /, parent() is None, so this stats . and can report / as not a mountpoint.
same for foo/.. or a symlink, where the lexical parent is wrong.
could you please stat path.join("..") instead?
|
|
||
| let path = at.plus_as_string("not-a-mountpoint"); | ||
|
|
||
| ucmd.current_dir("/proc/self") |
There was a problem hiding this comment.
do we need /proc/self? any cwd outside the fixture should be enough, no?
| .fails() | ||
| .code_is(32) | ||
| .stdout_is(format!("{path} is not a mountpoint\n")) | ||
| .no_stderr(); |
There was a problem hiding this comment.
please also add tests for / (is a mountpoint) and for a missing path (exit code + stderr)
Summary
Fixes
mountpointso it compares the target path with the target path's parent instead of the process working directory's parent.Motivation
The previous check could report regular directories such as
/etcas mount points when the current working directory lived on a different filesystem. util-linux reports non-mountpoints with exit status 32, so this also returns that status for non-mountpoint paths.Fixes #366.
Testing
cargo test test_mountpoint --features mountpointcargo clippy --all-targets --features mountpointcargo fmt --checkcargo testNote:
cargo clippy --all-targets --all-featuresstill fails on currentmainbecauseuudocis missing direct dependency declarations forzipanduuhelp_parser; that is handled separately in #546.