From 0bdf62d93a8fb274c3c401c587e6e76778c719dd Mon Sep 17 00:00:00 2001 From: "Manu [tennox]" <2084639+tennox@users.noreply.github.com> Date: Wed, 17 May 2023 10:14:59 +0100 Subject: [PATCH 1/3] #191 Add --no-swap flag --- src/cli.rs | 4 +++ src/main.rs | 1 + src/replacer.rs | 66 ++++++++++++++++++++++++++++++++----------------- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 862919e..9290436 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -55,6 +55,10 @@ w - match full words only /// use captured values like $1, $2, etc. pub replace_with: String, + #[arg(long)] + /// Overwrite file instead of creating tmp file and swaping atomically + pub no_swap: bool, + /// The path to file(s). This is optional - sd can also read from STDIN. ///{n}{n}Note: sd modifies files in-place by default. See documentation for /// examples. diff --git a/src/main.rs b/src/main.rs index de24644..97abb9d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,6 +30,7 @@ fn main() -> Result<()> { options.literal_mode, options.flags, options.replacements, + options.no_swap, )?, ) .run(options.preview)?; diff --git a/src/replacer.rs b/src/replacer.rs index f6d5d21..57f45d4 100644 --- a/src/replacer.rs +++ b/src/replacer.rs @@ -1,12 +1,18 @@ use crate::{utils, Error, Result}; use regex::bytes::Regex; -use std::{fs, fs::File, io::prelude::*, path::Path}; +use std::{ + fs, + fs::{File, OpenOptions}, + io::{prelude::*, SeekFrom}, + path::Path, +}; pub(crate) struct Replacer { regex: Regex, replace_with: Vec, is_literal: bool, replacements: usize, + no_swap: bool, } impl Replacer { @@ -16,6 +22,7 @@ impl Replacer { is_literal: bool, flags: Option, replacements: Option, + no_swap: bool, ) -> Result { let (look_for, replace_with) = if is_literal { (regex::escape(&look_for), replace_with.into_bytes()) @@ -61,6 +68,7 @@ impl Replacer { replace_with, is_literal, replacements: replacements.unwrap_or(0), + no_swap, }) } @@ -128,29 +136,42 @@ impl Replacer { return Ok(()); } - let source = File::open(path)?; - let meta = fs::metadata(path)?; - let mmap_source = unsafe { Mmap::map(&source)? }; - let replaced = self.replace(&mmap_source); - - let target = tempfile::NamedTempFile::new_in( - path.parent() - .ok_or_else(|| Error::InvalidPath(path.to_path_buf()))?, - )?; - let file = target.as_file(); - file.set_len(replaced.len() as u64)?; - file.set_permissions(meta.permissions())?; - - if !replaced.is_empty() { - let mut mmap_target = unsafe { MmapMut::map_mut(file)? }; - mmap_target.deref_mut().write_all(&replaced)?; - mmap_target.flush_async()?; - } + if self.no_swap { + let mut source = + OpenOptions::new().read(true).write(true).open(path)?; + let mut buffer = Vec::new(); + source.read_to_end(&mut buffer)?; + + let replaced = self.replace(&buffer); + + source.seek(SeekFrom::Start(0))?; + source.write_all(&replaced)?; + source.set_len(replaced.len() as u64)?; + } else { + let source = File::open(path)?; + let meta = fs::metadata(path)?; + let mmap_source = unsafe { Mmap::map(&source)? }; + let replaced = self.replace(&mmap_source); + + let target = tempfile::NamedTempFile::new_in( + path.parent() + .ok_or_else(|| Error::InvalidPath(path.to_path_buf()))?, + )?; + let file = target.as_file(); + file.set_len(replaced.len() as u64)?; + file.set_permissions(meta.permissions())?; + + if !replaced.is_empty() { + let mut mmap_target = unsafe { MmapMut::map_mut(file)? }; + mmap_target.deref_mut().write_all(&replaced)?; + mmap_target.flush_async()?; + } - drop(mmap_source); - drop(source); + drop(mmap_source); + drop(source); + target.persist(fs::canonicalize(path)?)?; + } - target.persist(fs::canonicalize(path)?)?; Ok(()) } } @@ -173,6 +194,7 @@ mod tests { literal, flags.map(ToOwned::to_owned), None, + false, ) .unwrap(); assert_eq!( From ca55e8cd3b88a4afa7f62925a48a6cee26b644ac Mon Sep 17 00:00:00 2001 From: "Manu [tennox]" <2084639+tennox@users.noreply.github.com> Date: Wed, 17 May 2023 10:31:14 +0100 Subject: [PATCH 2/3] #191 parameterize tests to include no-swap flag --- Cargo.toml | 1 + tests/cli.rs | 78 +++++++++++++++++++++++++++++++++------------------- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d39183d..b133801 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ clap = { version = "4.2.7", features = ["derive", "deprecated", "wrap_help"] } [dev-dependencies] assert_cmd = "1.0.3" anyhow = "1.0.38" +rstest = "0.17.0" [build-dependencies] clap = "4.2.7" diff --git a/tests/cli.rs b/tests/cli.rs index 29756b3..e36486e 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -3,6 +3,7 @@ mod cli { use anyhow::Result; use assert_cmd::Command; + use rstest::rstest; use std::io::prelude::*; fn sd() -> Command { @@ -25,36 +26,46 @@ mod cli { Ok(()) } - #[test] - fn in_place() -> Result<()> { + #[rstest] + fn in_place(#[values(false, true)] no_swap: bool) -> Result<()> { let mut file = tempfile::NamedTempFile::new()?; file.write_all(b"abc123def")?; let path = file.into_temp_path(); - sd().args(["abc\\d+", "", path.to_str().unwrap()]) - .assert() - .success(); + let mut cmd = sd(); + cmd.args(["abc\\d+", "", path.to_str().unwrap()]); + if no_swap { + cmd.arg("--no-swap"); + } + cmd.assert().success(); assert_file(&path, "def"); Ok(()) } - #[test] - fn in_place_with_empty_result_file() -> Result<()> { + #[rstest] + fn in_place_with_empty_result_file( + #[values(false, true)] no_swap: bool, + ) -> Result<()> { let mut file = tempfile::NamedTempFile::new()?; file.write_all(b"a7c")?; let path = file.into_temp_path(); - sd().args(["a\\dc", "", path.to_str().unwrap()]) - .assert() - .success(); + let mut cmd = sd(); + cmd.args(["a\\dc", "", path.to_str().unwrap()]); + if no_swap { + cmd.arg("--no-swap"); + } + cmd.assert().success(); assert_file(&path, ""); Ok(()) } - #[test] - fn in_place_following_symlink() -> Result<()> { + #[rstest] + fn in_place_following_symlink( + #[values(false, true)] no_swap: bool, + ) -> Result<()> { let dir = tempfile::tempdir()?; let path = dir.path(); let file = path.join("file"); @@ -63,9 +74,12 @@ mod cli { create_soft_link(&file, &link)?; std::fs::write(&file, "abc123def")?; - sd().args(["abc\\d+", "", link.to_str().unwrap()]) - .assert() - .success(); + let mut cmd = sd(); + cmd.args(["abc\\d+", "", link.to_str().unwrap()]); + if no_swap { + cmd.arg("--no-swap"); + } + cmd.assert().success(); assert_file(&file, "def"); assert!(std::fs::symlink_metadata(link)?.file_type().is_symlink()); @@ -73,29 +87,35 @@ mod cli { Ok(()) } - #[test] - fn replace_into_stdout() -> Result<()> { + #[rstest] + fn replace_into_stdout(#[values(false, true)] no_swap: bool) -> Result<()> { let mut file = tempfile::NamedTempFile::new()?; file.write_all(b"abc123def")?; - sd().args(["-p", "abc\\d+", "", file.path().to_str().unwrap()]) - .assert() - .success() - .stdout(format!( - "{}{}def\n", - ansi_term::Color::Green.prefix(), - ansi_term::Color::Green.suffix() - )); + let mut cmd = sd(); + cmd.args(["-p", "abc\\d+", "", file.path().to_str().unwrap()]); + if no_swap { + cmd.arg("--no-swap"); + } + cmd.assert().success().stdout(format!( + "{}{}def\n", + ansi_term::Color::Green.prefix(), + ansi_term::Color::Green.suffix() + )); assert_file(file.path(), "abc123def"); Ok(()) } - #[test] - fn stdin() -> Result<()> { - sd().args(["abc\\d+", ""]) - .write_stdin("abc123def") + #[rstest] + fn stdin(#[values(false, true)] no_swap: bool) -> Result<()> { + let mut cmd = sd(); + cmd.args(["abc\\d+", ""]); + if no_swap { + cmd.arg("--no-swap"); + } + cmd.write_stdin("abc123def") .assert() .success() .stdout("def"); From b7248bbf18c82f6817079b41eb4df2df59570bc2 Mon Sep 17 00:00:00 2001 From: "Manu [tennox]" Date: Fri, 10 May 2024 17:03:56 +0100 Subject: [PATCH 3/3] Rename flag to --in-place as per suggestion https://github.com/chmln/sd/pull/193#issuecomment-1847964368 --- src/cli.rs | 4 ++-- src/main.rs | 6 +++--- tests/cli.rs | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 586de4f..9a4eeac 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -65,8 +65,8 @@ w - match full words only pub replace_with: String, #[arg(long)] - /// Overwrite file instead of creating tmp file and swaping atomically - pub no_swap: bool, + /// Overwrite file in place instead of creating tmp file and swapping atomically + pub in_place: bool, /// The path to file(s). This is optional - sd can also read from STDIN. /// diff --git a/src/main.rs b/src/main.rs index 5791fb7..b7b493f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -91,10 +91,10 @@ fn try_main() -> Result<()> { for (source, replaced) in sources.iter().zip(replaced) { match source { Source::File(path) => { - let result = if !options.no_swap { + let result = if !options.in_place { write_with_temp(path, &replaced) } else { - write_without_temp(path, &replaced) + write_in_place(path, &replaced) }; if let Err(e) = result { failed_jobs.push((path.to_owned(), e)); @@ -136,7 +136,7 @@ fn write_with_temp(path: &PathBuf, data: &[u8]) -> Result<()> { Ok(()) } -fn write_without_temp(path: &PathBuf, data: &[u8]) -> Result<()> { +fn write_in_place(path: &PathBuf, data: &[u8]) -> Result<()> { let path = fs::canonicalize(path)?; let mut source = fs::OpenOptions::new().write(true).open(path)?; diff --git a/tests/cli.rs b/tests/cli.rs index 7028b8e..5d06726 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -39,7 +39,7 @@ mod cli { let mut cmd = sd(); cmd.args(["abc\\d+", "", path.to_str().unwrap()]); if no_swap { - cmd.arg("--no-swap"); + cmd.arg("--in-place"); } cmd.assert().success(); assert_file(&path, "def"); @@ -58,7 +58,7 @@ mod cli { let mut cmd = sd(); cmd.args(["a\\dc", "", path.to_str().unwrap()]); if no_swap { - cmd.arg("--no-swap"); + cmd.arg("--in-place"); } cmd.assert().success(); assert_file(&path, ""); @@ -85,7 +85,7 @@ mod cli { let mut cmd = sd(); cmd.args(["abc\\d+", "", link.to_str().unwrap()]); if no_swap { - cmd.arg("--no-swap"); + cmd.arg("--in-place"); } cmd.assert().success(); @@ -103,7 +103,7 @@ mod cli { let mut cmd = sd(); cmd.args(["-p", "abc\\d+", "", file.path().to_str().unwrap()]); if no_swap { - cmd.arg("--no-swap"); + cmd.arg("--in-place"); } cmd.assert().success().stdout("def"); @@ -117,7 +117,7 @@ mod cli { let mut cmd = sd(); cmd.args(["abc\\d+", ""]); if no_swap { - cmd.arg("--no-swap"); + cmd.arg("--in-place"); } cmd.write_stdin("abc123def") .assert()