Retain ownership on atomic writes

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<PathBuf>) -> Vec<Self> {
-        paths.into_iter().map(Self::File).collect()
+    pub(crate) fn from_paths(paths: Vec<PathBuf>) -> Result<Vec<Self>> {
+        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<Self> {
@@ -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<Mmap> {
-    Ok(Mmap::map(&File::open(path)?)?)
+pub(crate) fn make_mmap(path: &PathBuf) -> Result<Mmap> {
+    Ok(unsafe { Mmap::map(&File::open(path)?)? })
 }
 
 pub(crate) fn make_mmap_stdin() -> Result<Mmap> {

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,38 +36,31 @@ 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::<Result<Vec<_>>>()?;
 
     let replaced: Vec<_> = {
         use rayon::prelude::*;
         mmaps
             .par_iter()
-            .map(|mmap| replacer.replace(&mmap))
+            .map(|mmap| replacer.replace(mmap))
             .collect()
     };
 
     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,46 +78,21 @@ 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::<Vec<_>>();
+
         if !failed_jobs.is_empty() {
             return Err(Error::FailedJobs(FailedJobs(failed_jobs)));
         }
     }
 
     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(())
-}

src/output.rs

@@ -0,0 +1,37 @@
+use crate::{Error, Result};
+use memmap2::MmapMut;
+use std::{fs, io::Write, ops::DerefMut, 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::fs::{MetadataExt, fchown};
+            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(())
+}

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<Capture<'_>> {
     }
 
     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<Capture<'_>> {
     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;
     }
 

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<char> {
     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)

xtask/src/gen.rs → 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);

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(),
     }
 }