forked from extern/nushell
Fix missing file names from rm errors (#9120)
# Description Fixes a small bug with `rm` where names of files which couldn't be deleted due to error were not printed. Fixes https://github.com/nushell/nushell/issues/9004 # User-Facing Changes Slightly different error message than previously. Nothing significant, though. The new error message looks like this ``` ~/Projects/rust/nushell> rm /proc/1/mem 05/06/2023 01:13:23 PM Error: nu:🐚:remove_not_possible × Remove not possible ╭─[entry #3:1:1] 1 │ rm /proc/1/mem · ─────┬───── · ╰── Could not delete /proc/1/mem: Operation not permitted (os error 1) ╰──── ``` or when using a glob (only showing a single entry for brevity) ``` Error: nu:🐚:remove_not_possible × Remove not possible ╭─[entry #2:1:1] 1 │ rm --recursive --force --verbose /proc/1/* · ────┬──── · ╰── Could not delete /proc/1/comm: Operation not permitted (os error 1) ╰──── ``` # Tests + Formatting No new unit tests were added for this change as it is pretty difficult to test this particular case. However, manual testing was run with the following commands ``` rm /proc/1/mem rm --recursive --force --verbose /proc/1/* ``` # After Submitting N/A
This commit is contained in:
parent
6c730def4b
commit
c12b211075
@ -419,15 +419,9 @@ fn rm(
|
||||
}
|
||||
|
||||
if let Err(e) = result {
|
||||
let msg = format!("Could not delete because: {e:}");
|
||||
let msg = format!("Could not delete {:}: {e:}", f.to_string_lossy());
|
||||
Value::Error {
|
||||
error: Box::new(ShellError::GenericError(
|
||||
msg,
|
||||
e.to_string(),
|
||||
Some(span),
|
||||
None,
|
||||
Vec::new(),
|
||||
)),
|
||||
error: Box::new(ShellError::RemoveNotPossible(msg, span)),
|
||||
}
|
||||
} else if verbose {
|
||||
let msg = if interactive && !confirmed {
|
||||
|
@ -1,6 +1,8 @@
|
||||
use nu_test_support::fs::{files_exist_at, Stub::EmptyFile};
|
||||
use nu_test_support::nu;
|
||||
use nu_test_support::playground::Playground;
|
||||
use std::fs;
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[test]
|
||||
fn removes_a_file() {
|
||||
@ -349,3 +351,60 @@ fn remove_ignores_ansi() {
|
||||
assert_eq!(actual.out, "true");
|
||||
});
|
||||
}
|
||||
|
||||
struct Cleanup<'a> {
|
||||
dir_to_clean: &'a PathBuf,
|
||||
}
|
||||
|
||||
fn set_dir_read_only(directory: &PathBuf, read_only: bool) {
|
||||
let mut permissions = fs::metadata(directory).unwrap().permissions();
|
||||
permissions.set_readonly(read_only);
|
||||
fs::set_permissions(directory, permissions).expect("failed to set directory permissions");
|
||||
}
|
||||
|
||||
impl<'a> Drop for Cleanup<'a> {
|
||||
/// Restores write permissions to the given directory so that the Playground can be successfully
|
||||
/// cleaned up.
|
||||
fn drop(&mut self) {
|
||||
set_dir_read_only(self.dir_to_clean, false);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
// This test is only about verifying file names are included in rm error messages. It is easier
|
||||
// to only have this work on non-windows systems (i.e., unix-like) than to try to get the
|
||||
// permissions to work on all platforms.
|
||||
#[cfg(not(windows))]
|
||||
fn rm_prints_filenames_on_error() {
|
||||
Playground::setup("rm_prints_filenames_on_error", |dirs, sandbox| {
|
||||
let file_names = vec!["test1.txt", "test2.txt"];
|
||||
|
||||
let with_files = file_names
|
||||
.iter()
|
||||
.map(|file_name| EmptyFile(file_name))
|
||||
.collect();
|
||||
sandbox.with_files(with_files);
|
||||
|
||||
let test_dir = dirs.test();
|
||||
|
||||
set_dir_read_only(test_dir, true);
|
||||
let _cleanup = Cleanup {
|
||||
dir_to_clean: &test_dir,
|
||||
};
|
||||
|
||||
// This rm is expected to fail, and stderr output indicating so is also expected.
|
||||
let actual = nu!(cwd: test_dir, "rm test*.txt");
|
||||
|
||||
assert!(files_exist_at(file_names.clone(), test_dir));
|
||||
for file_name in file_names {
|
||||
let path = test_dir.join(file_name);
|
||||
let substr = format!("Could not delete {}", path.to_string_lossy());
|
||||
assert!(
|
||||
actual.err.contains(&substr),
|
||||
"Matching: {}\n=== Command stderr:\n{}\n=== End stderr",
|
||||
substr,
|
||||
actual.err
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
@ -869,9 +869,12 @@ pub enum ShellError {
|
||||
ChangeModifiedTimeNotPossible(String, #[label("{0}")] Span),
|
||||
|
||||
/// Unable to remove this item.
|
||||
///
|
||||
/// ## Resolution
|
||||
///
|
||||
/// Removal can fail for a number of reasons, such as permissions problems. Refer to the specific error message for more details.
|
||||
#[error("Remove not possible")]
|
||||
#[diagnostic(code(nu::shell::remove_not_possible))]
|
||||
// NOTE: Currently unused. Remove?
|
||||
RemoveNotPossible(String, #[label("{0}")] Span),
|
||||
|
||||
// These three are unused. Remove?
|
||||
|
Loading…
Reference in New Issue
Block a user