From 83720a9f302a2672990ae09fad5b501cde54d911 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Mon, 22 Apr 2024 01:12:13 +0000 Subject: [PATCH] Make the same file error more likely to appear (#12601) # 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. --- crates/nu-command/src/filesystem/save.rs | 87 +++++++++++++++--------- crates/nu-command/src/filters/lines.rs | 10 ++- crates/nu-command/tests/commands/save.rs | 46 ++++++++++++- 3 files changed, 104 insertions(+), 39 deletions(-) diff --git a/crates/nu-command/src/filesystem/save.rs b/crates/nu-command/src/filesystem/save.rs index 0dec55a4ef..ba8fae4a90 100644 --- a/crates/nu-command/src/filesystem/save.rs +++ b/crates/nu-command/src/filesystem/save.rs @@ -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) -> 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, + stderr_dest: Option<&Spanned>, +) -> 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( diff --git a/crates/nu-command/src/filters/lines.rs b/crates/nu-command/src/filters/lines.rs index 53360001db..0a07378afb 100644 --- a/crates/nu-command/src/filters/lines.rs +++ b/crates/nu-command/src/filters/lines.rs @@ -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)), } } diff --git a/crates/nu-command/tests/commands/save.rs b/crates/nu-command/tests/commands/save.rs index 9febfdc2e7..6739cd88fe 100644 --- a/crates/nu-command/tests/commands/save.rs +++ b/crates/nu-command/tests/commands/save.rs @@ -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")); }) }