mv fixes and tests
#78
Merged
Conversation
This file contains 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
jgarzik
reviewed
Apr 27, 2024
tree/src/mv.rs
Outdated
| // 1. If the destination path exists, conditionally prompt user | ||
| let target_md = match fs::metadata(target) { | ||
| Ok(md) => Some(md), | ||
| Err(e) => { | ||
| if e.kind() == io::ErrorKind::NotFound { | ||
| None | ||
| } else { | ||
| eprintln!("{}: {}", target, e); | ||
| eprintln!("{}: {}", target.to_string_lossy(), e); |
There was a problem hiding this comment.
Prefer to standardize on .display() for outputting Path and PathBuf: https://doc.rust-lang.org/std/path/struct.Path.html#method.display
jgarzik
reviewed
Apr 27, 2024
tree/src/mv.rs
Outdated
| let is_affirm = prompt_user(&format!("{}: {}", target, gettext("overwrite?"))); | ||
| let is_affirm = prompt_user(&format!( | ||
| "{}: {}", | ||
| target.to_string_lossy(), |
jgarzik
reviewed
Apr 27, 2024
tree/src/mv.rs
Outdated
| } | ||
| (false, true) => { | ||
| let err_str = "cannot overwrite directory with non-directory"; | ||
| eprintln!("{}: {}", target.to_string_lossy(), gettext(err_str)); |
|
general comment: it is preferred to create a separate commit that moves |
JRF63
force-pushed
the
mv-fixes
branch
2 times, most recently
from
1a9af94 to
16b2464
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
This pretty much has max compatibility with GNU coreutils mv even down to the error messages.
There's 3 tests locked under the
posixutils_test_allfeature flag:test_mv_hardlink_case- Needs a case-insensitive filesystem that can support hard links. It's hard to do it dynamically so it must be supplied through an env var,CASE_INSENSITIVE_TMPDIRtest_mv_i_link_no- This requires script to fake ttytest_mv_sticky_to_xpart- Needs root, a username inNON_ROOT_USERNAMEand must not be run together with other testsI experimented enabling the
posixutils_test_allfeature dynamically based on an environment variable using a build script like in this method but it seems to mess-up with incremental linking and forces a recompile everytime. It's possible that it was due to the usage ofoption_env!andstd::env::varmight not have the same problems.The tests requiring
/dev/shmcan be made to work in macOS if a directory in another filesystem is supplied inOTHER_PARTITION_TMPDIR. Haven't tested if it's possible to calldiskutilornewfsfrom the GH macOS runner.