Mirrors/nushell
2
0
Fork 0
mirror of https://github.com/nushell/nushell synced 2024-09-20 14:42:01 +00:00
Code Issues Projects Releases Packages Wiki Activity

Make the same file error more likely to appear (#12601)

Browse source 
# Description
When saving to a file we currently try to check if the data source in
the pipeline metadata is the same as the file we are saving to. If so,
we create an error, since reading and writing to a file at the same time
is currently not supported/handled gracefully. However, there are still
a few instances where this error is not properly triggered, and so this
PR attempts to reduce these cases. Inspired by #12599.

# Tests + Formatting
Added a few tests.

# After Submitting
Some commands still do not properly preserve metadata (e.g., `str trim`)
and so prevent us from detecting this error.
This commit is contained in:
Ian Manske 2024-04-22 01:12:13 +00:00 committed by GitHub
parent a60381a932
commit 83720a9f30
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 104 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

87
crates/nu-command/src/filesystem/save.rs
View file

@ -101,7 +101,14 @@ impl Command for Save {
});
match input {
PipelineData::ExternalStream { stdout, stderr, .. } => {
PipelineData::ExternalStream {
stdout,
stderr,
metadata,
..
} => {
check_saving_to_source_file(metadata.as_ref(), &path, stderr_path.as_ref())?;
let (file, stderr_file) = get_files(
&path,
stderr_path.as_ref(),
@ -151,38 +158,11 @@ impl Command for Save {
PipelineData::ListStream(ls, pipeline_metadata)
if raw || prepare_path(&path, append, force)?.0.extension().is_none() =>
{
if let Some(PipelineMetadata {
data_source: DataSource::FilePath(input_path),
}) = pipeline_metadata
{
if path.item == input_path {
return Err(ShellError::GenericError {
error: "pipeline input and output are same file".into(),
msg: format!(
"can't save output to '{}' while it's being reading",
path.item.display()
),
span: Some(path.span),
help: Some("you should change output path".into()),
inner: vec![],
});
}
if let Some(ref err_path) = stderr_path {
if err_path.item == input_path {
return Err(ShellError::GenericError {
error: "pipeline input and stderr are same file".into(),
msg: format!(
"can't save stderr to '{}' while it's being reading",
err_path.item.display()
),
span: Some(err_path.span),
help: Some("you should change stderr path".into()),
inner: vec![],
});
}
}
}
check_saving_to_source_file(
pipeline_metadata.as_ref(),
&path,
stderr_path.as_ref(),
)?;
let (mut file, _) = get_files(
&path,
@ -207,6 +187,12 @@ impl Command for Save {
Ok(PipelineData::empty())
}
input => {
check_saving_to_source_file(
input.metadata().as_ref(),
&path,
stderr_path.as_ref(),
)?;
let bytes =
input_to_bytes(input, Path::new(&path.item), raw, engine_state, stack, span)?;
@ -266,6 +252,41 @@ impl Command for Save {
}
}
fn saving_to_source_file_error(dest: &Spanned<PathBuf>) -> ShellError {
ShellError::GenericError {
error: "pipeline input and output are the same file".into(),
msg: format!(
"can't save output to '{}' while it's being read",
dest.item.display()
),
span: Some(dest.span),
help: Some("you should change the output path".into()),
inner: vec![],
}
}
fn check_saving_to_source_file(
metadata: Option<&PipelineMetadata>,
dest: &Spanned<PathBuf>,
stderr_dest: Option<&Spanned<PathBuf>>,
) -> Result<(), ShellError> {
let Some(DataSource::FilePath(source)) = metadata.map(|meta| &meta.data_source) else {
return Ok(());
};
if &dest.item == source {
return Err(saving_to_source_file_error(dest));
}
if let Some(dest) = stderr_dest {
if &dest.item == source {
return Err(saving_to_source_file_error(dest));
}
}
Ok(())
}
/// Convert [`PipelineData`] bytes to write in file, possibly converting
/// to format of output file
fn input_to_bytes(

10
crates/nu-command/src/filters/lines.rs
View file

@ -51,7 +51,7 @@ impl Command for Lines {
Ok(Value::list(lines, span).into_pipeline_data())
}
PipelineData::Empty => Ok(PipelineData::Empty),
PipelineData::ListStream(stream, ..) => {
PipelineData::ListStream(stream, metadata) => {
let iter = stream
.into_iter()
.filter_map(move |value| {
@ -74,7 +74,9 @@ impl Command for Lines {
})
.flatten();
Ok(iter.into_pipeline_data(engine_state.ctrlc.clone()))
Ok(iter
.into_pipeline_data(engine_state.ctrlc.clone())
.set_metadata(metadata))
}
PipelineData::Value(val, ..) => {
match val {
@ -91,10 +93,12 @@ impl Command for Lines {
PipelineData::ExternalStream { stdout: None, .. } => Ok(PipelineData::empty()),
PipelineData::ExternalStream {
stdout: Some(stream),
metadata,
..
} => Ok(RawStreamLinesAdapter::new(stream, head, skip_empty)
.map(move |x| x.unwrap_or_else(|err| Value::error(err, head)))
.into_pipeline_data(ctrlc)),
.into_pipeline_data(ctrlc)
.set_metadata(metadata)),
}
}

46
crates/nu-command/tests/commands/save.rs
View file

@ -329,6 +329,26 @@ fn save_file_correct_relative_path() {
#[test]
fn save_same_file_with_extension() {
Playground::setup("save_test_16", |dirs, _sandbox| {
let actual = nu!(
cwd: dirs.test(), pipeline(
"
echo 'world'
| save --raw hello.md;
open --raw hello.md
| save --raw --force hello.md
"
)
);
assert!(actual
.err
.contains("pipeline input and output are the same file"));
})
}
#[test]
fn save_same_file_with_extension_pipeline() {
Playground::setup("save_test_17", |dirs, _sandbox| {
let actual = nu!(
cwd: dirs.test(), pipeline(
"
@ -343,13 +363,33 @@ fn save_same_file_with_extension() {
assert!(actual
.err
.contains("pipeline input and output are same file"));
.contains("pipeline input and output are the same file"));
})
}
#[test]
fn save_same_file_without_extension() {
Playground::setup("save_test_17", |dirs, _sandbox| {
Playground::setup("save_test_18", |dirs, _sandbox| {
let actual = nu!(
cwd: dirs.test(), pipeline(
"
echo 'world'
| save hello;
open hello
| save --force hello
"
)
);
assert!(actual
.err
.contains("pipeline input and output are the same file"));
})
}
#[test]
fn save_same_file_without_extension_pipeline() {
Playground::setup("save_test_19", |dirs, _sandbox| {
let actual = nu!(
cwd: dirs.test(), pipeline(
"
@ -364,6 +404,6 @@ fn save_same_file_without_extension() {
assert!(actual
.err
.contains("pipeline input and output are same file"));
.contains("pipeline input and output are the same file"));
})
}