From bffe27154cd3de03f343ca6941bb60ba951113c2 Mon Sep 17 00:00:00 2001 From: Gregory Date: Fri, 19 Dec 2025 21:22:35 -0500 Subject: [PATCH 1/3] xtask - clean up --- xtask/src/{gen.rs => generate.rs} | 6 +++--- xtask/src/main.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) rename xtask/src/{gen.rs => generate.rs} (97%) diff --git a/xtask/src/gen.rs b/xtask/src/generate.rs similarity index 97% rename from xtask/src/gen.rs rename to xtask/src/generate.rs index 7f22d3f..c89e2cc 100644 --- a/xtask/src/gen.rs +++ b/xtask/src/generate.rs @@ -6,10 +6,10 @@ use sd::Options; use std::{fs, path::Path}; use clap::{CommandFactory, ValueEnum}; -use clap_complete::{generate_to, Shell}; -use roff::{bold, roman, Roff}; +use clap_complete::{Shell, generate_to}; +use roff::{Roff, bold, roman}; -pub fn gen() { +pub fn generate() { let gen_dir = Path::new("gen"); gen_shell(gen_dir); gen_man(gen_dir); diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 081c7c2..1f22f28 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -5,7 +5,7 @@ use std::{ use clap::{Parser, Subcommand}; -mod gen; +mod generate; #[derive(Parser)] struct Cli { @@ -25,7 +25,7 @@ fn main() { env::set_current_dir(project_root()).unwrap(); match command { - Commands::Gen => gen::gen(), + Commands::Gen => generate::generate(), } } From 553663c3a73c8e507a27598fbae34a0fbc877841 Mon Sep 17 00:00:00 2001 From: Gregory Date: Fri, 19 Dec 2025 21:26:25 -0500 Subject: [PATCH 2/3] Retain file ownership on atomic writes (#326) Extract atomic write logic into output module and preserve original file uid/gid when replacing files via tempfile. Also refactors path validation to fail early and reduces unsafe scope in mmap creation. --- src/input.rs | 21 +++++++++---- src/main.rs | 84 +++++++++++++++------------------------------------ src/output.rs | 43 ++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 66 deletions(-) create mode 100644 src/output.rs diff --git a/src/input.rs b/src/input.rs index fb98e62..3d74f85 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,11 +1,11 @@ use memmap2::{Mmap, MmapOptions}; use std::{ fs::File, - io::{stdin, Read}, + io::{Read, stdin}, path::PathBuf, }; -use crate::error::Result; +use crate::error::{Error, Result}; #[derive(Debug, PartialEq)] pub(crate) enum Source { @@ -14,8 +14,17 @@ pub(crate) enum Source { } impl Source { - pub(crate) fn from_paths(paths: Vec) -> Vec { - paths.into_iter().map(Self::File).collect() + pub(crate) fn from_paths(paths: Vec) -> Result> { + paths + .into_iter() + .map(|path| { + if path.exists() { + Ok(Source::File(path)) + } else { + Err(Error::InvalidPath(path.clone())) + } + }) + .collect() } pub(crate) fn from_stdin() -> Vec { @@ -33,8 +42,8 @@ impl Source { // TODO: memmap2 docs state that users should implement proper // procedures to avoid problems the `unsafe` keyword indicate. // This would be in a later PR. -pub(crate) unsafe fn make_mmap(path: &PathBuf) -> Result { - Ok(Mmap::map(&File::open(path)?)?) +pub(crate) fn make_mmap(path: &PathBuf) -> Result { + Ok(unsafe { Mmap::map(&File::open(path)?)? }) } pub(crate) fn make_mmap_stdin() -> Result { diff --git a/src/main.rs b/src/main.rs index 594e796..58d9074 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,18 +1,14 @@ -#![feature(try_blocks)] mod cli; mod error; mod input; +mod output; pub(crate) mod replacer; mod unescape; use clap::Parser; -use memmap2::MmapMut; use std::{ - fs, - io::{stdout, Write}, - ops::DerefMut, - path::PathBuf, + io::{Write, stdout}, process, }; @@ -40,28 +36,20 @@ fn try_main() -> Result<()> { )?; let sources = if !options.files.is_empty() { - Source::from_paths(options.files) + Source::from_paths(options.files)? } else { Source::from_stdin() }; - let mut mmaps = Vec::new(); - for source in sources.iter() { - let mmap = match source { - Source::File(path) => { - if path.exists() { - unsafe { make_mmap(&path)? } - } else { - return Err(Error::InvalidPath(path.to_owned())); - } - } - Source::Stdin => make_mmap_stdin()?, - }; - - mmaps.push(mmap); - } - - let needs_separator = sources.len() > 1; + let mmaps = sources + .iter() + .map(|source| { + Ok(match source { + Source::File(path) => make_mmap(&path)?, + Source::Stdin => make_mmap_stdin()?, + }) + }) + .collect::>>()?; let replaced: Vec<_> = { use rayon::prelude::*; @@ -72,6 +60,7 @@ fn try_main() -> Result<()> { }; if options.preview || sources.first() == Some(&Source::Stdin) { + let needs_separator = sources.len() > 1; let mut handle = stdout().lock(); for (source, replaced) in sources.iter().zip(replaced) { @@ -89,17 +78,17 @@ fn try_main() -> Result<()> { #[cfg(target_family = "windows")] drop(mmaps); - let mut failed_jobs = Vec::new(); - for (source, replaced) in sources.iter().zip(replaced) { - match source { - Source::File(path) => { - if let Err(e) = write_with_temp(path, &replaced) { - failed_jobs.push((path.to_owned(), e)); - } - } - _ => unreachable!("stdin should go previous branch"), - } - } + let failed_jobs = sources + .iter() + .zip(replaced) + .filter_map(|(source, replaced)| match source { + Source::File(path) => output::write_atomic(path, &replaced) + .err() + .map(|e| (path.to_owned(), e)), + _ => None, + }) + .collect::>(); + if !failed_jobs.is_empty() { return Err(Error::FailedJobs(FailedJobs(failed_jobs))); } @@ -107,28 +96,3 @@ fn try_main() -> Result<()> { Ok(()) } - -fn write_with_temp(path: &PathBuf, data: &[u8]) -> Result<()> { - let path = fs::canonicalize(path)?; - - let temp = tempfile::NamedTempFile::new_in( - path.parent() - .ok_or_else(|| Error::InvalidPath(path.to_path_buf()))?, - )?; - - let file = temp.as_file(); - file.set_len(data.len() as u64)?; - if let Ok(metadata) = fs::metadata(&path) { - file.set_permissions(metadata.permissions()).ok(); - } - - if !data.is_empty() { - let mut mmap_temp = unsafe { MmapMut::map_mut(file)? }; - mmap_temp.deref_mut().write_all(data)?; - mmap_temp.flush_async()?; - } - - temp.persist(&path)?; - - Ok(()) -} diff --git a/src/output.rs b/src/output.rs new file mode 100644 index 0000000..758b205 --- /dev/null +++ b/src/output.rs @@ -0,0 +1,43 @@ +use crate::{Error, Result}; +use memmap2::MmapMut; +use std::{ + fs, io::Write, ops::DerefMut, os::unix::fs::MetadataExt, path::Path, +}; + +pub(crate) fn write_atomic(path: &Path, data: &[u8]) -> Result<()> { + let path = fs::canonicalize(path)?; + + let temp = tempfile::NamedTempFile::new_in( + path.parent() + .ok_or_else(|| Error::InvalidPath(path.to_path_buf()))?, + )?; + + let file = temp.as_file(); + file.set_len(data.len() as u64)?; + + if let Ok(metadata) = fs::metadata(&path) { + file.set_permissions(metadata.permissions()).ok(); + + // Explicitly retain ownership + #[cfg(unix)] + { + use std::os::unix; + unix::fs::fchown( + &file, + Some(metadata.uid()), + Some(metadata.gid()), + )?; + metadata.gid(); + } + } + + if !data.is_empty() { + let mut mmap_temp = unsafe { MmapMut::map_mut(file)? }; + mmap_temp.deref_mut().write_all(data)?; + mmap_temp.flush_async()?; + } + + temp.persist(&path)?; + + Ok(()) +} From 84c7e6e810328c2091dfdd060e423455db561e50 Mon Sep 17 00:00:00 2001 From: Gregory Date: Fri, 19 Dec 2025 21:30:01 -0500 Subject: [PATCH 3/3] Fix clippy warnings --- src/main.rs | 4 ++-- src/output.rs | 12 +++--------- src/replacer/validate.rs | 15 +++++++-------- src/unescape.rs | 2 +- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/main.rs b/src/main.rs index 58d9074..a760ed9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -45,7 +45,7 @@ fn try_main() -> Result<()> { .iter() .map(|source| { Ok(match source { - Source::File(path) => make_mmap(&path)?, + Source::File(path) => make_mmap(path)?, Source::Stdin => make_mmap_stdin()?, }) }) @@ -55,7 +55,7 @@ fn try_main() -> Result<()> { use rayon::prelude::*; mmaps .par_iter() - .map(|mmap| replacer.replace(&mmap)) + .map(|mmap| replacer.replace(mmap)) .collect() }; diff --git a/src/output.rs b/src/output.rs index 758b205..5f3ec61 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,8 +1,6 @@ use crate::{Error, Result}; use memmap2::MmapMut; -use std::{ - fs, io::Write, ops::DerefMut, os::unix::fs::MetadataExt, path::Path, -}; +use std::{fs, io::Write, ops::DerefMut, path::Path}; pub(crate) fn write_atomic(path: &Path, data: &[u8]) -> Result<()> { let path = fs::canonicalize(path)?; @@ -21,12 +19,8 @@ pub(crate) fn write_atomic(path: &Path, data: &[u8]) -> Result<()> { // Explicitly retain ownership #[cfg(unix)] { - use std::os::unix; - unix::fs::fchown( - &file, - Some(metadata.uid()), - Some(metadata.gid()), - )?; + use std::os::unix::fs::{MetadataExt, fchown}; + fchown(file, Some(metadata.uid()), Some(metadata.gid()))?; metadata.gid(); } } diff --git a/src/replacer/validate.rs b/src/replacer/validate.rs index a7ce765..cc006d7 100644 --- a/src/replacer/validate.rs +++ b/src/replacer/validate.rs @@ -76,11 +76,11 @@ impl fmt::Display for InvalidReplaceCapture { }; if let Some(prefix) = prefix { - formatted.push_str(&prefix.to_string()); + formatted.push_str(prefix); } formatted.push(text); if let Some(suffix) = suffix { - formatted.push_str(&suffix.to_string()); + formatted.push_str(suffix); } if byte_index < invalid_ident.start { @@ -97,14 +97,13 @@ impl fmt::Display for InvalidReplaceCapture { // This relies on all non-curly-braced capture chars being 1 byte let arrows_span = arrows_start.end_offset(invalid_ident.len()); let mut arrows = " ".repeat(arrows_span.start); - arrows.push_str(&format!("{}", "^".repeat(arrows_span.len()))); + arrows.push_str(&"^".repeat(arrows_span.len())); let ident = invalid_ident.slice(original_replace); let (number, the_rest) = ident.split_at(*num_leading_digits); let disambiguous = format!("${{{number}}}{the_rest}"); let error_message = format!( - "The numbered capture group `{}` in the replacement text is ambiguous.", - format!("${}", number).to_string() + "The numbered capture group `${number}` in the replacement text is ambiguous.", ); let hint_message = format!( "{}: Use curly braces to disambiguate it `{}`.", @@ -252,7 +251,7 @@ fn find_cap_ref(rep: &[u8], open_span: SpanOpen) -> Option> { } let mut cap_end = 0; - while rep.get(cap_end).copied().map_or(false, is_valid_cap_letter) { + while rep.get(cap_end).copied().is_some_and(is_valid_cap_letter) { cap_end += 1; } if cap_end == 0 { @@ -274,10 +273,10 @@ fn find_cap_ref_braced(rep: &[u8], open_span: SpanOpen) -> Option> { assert_eq!(b'{', rep[0]); let mut cap_end = 1; - while rep.get(cap_end).map_or(false, |&b| b != b'}') { + while rep.get(cap_end).is_some_and(|&b| b != b'}') { cap_end += 1; } - if !rep.get(cap_end).map_or(false, |&b| b == b'}') { + if rep.get(cap_end).is_none_or(|&b| b != b'}') { return None; } diff --git a/src/unescape.rs b/src/unescape.rs index 63c1888..182de60 100644 --- a/src/unescape.rs +++ b/src/unescape.rs @@ -46,7 +46,7 @@ pub fn unescape(input: &str) -> String { /// This is for sequences such as `\x08` or `\u1234` fn escape_n_chars(chars: &mut Chars<'_>, length: usize) -> Option { let s = chars.as_str().get(0..length)?; - let u = u32::from_str_radix(&s, 16).ok()?; + let u = u32::from_str_radix(s, 16).ok()?; let ch = char::from_u32(u)?; _ = chars.nth(length); Some(ch)