From f6c993490287a5e61e9cd857f25553dcf8efc6cf Mon Sep 17 00:00:00 2001 From: Anthony DePasquale Date: Tue, 29 Sep 2026 15:23:51 +0200 Subject: [PATCH 1/2] sed: take the -i suffix only when attached, as GNU sed does --- docs/src/extensions.md | 2 + src/sed/mod.rs | 187 ++++++++++++++++++++++++++++++++++++-- tests/by-util/test_sed.rs | 81 ++++++++++++++--- 3 files changed, 253 insertions(+), 17 deletions(-) diff --git a/docs/src/extensions.md b/docs/src/extensions.md index 3b2a1adf..67360098 100644 --- a/docs/src/extensions.md +++ b/docs/src/extensions.md @@ -35,6 +35,8 @@ Below is a list of these extensions and incompatibilities. ## Supported BSD and GNU extensions * The second address in a range can be specified as a relative address with +N. * In-place editing of file with the `-i` flag. + As in GNU _sed_, a backup suffix must be attached (`-i.bak`, + `--in-place=.bak`); BSD's `-i .bak` and `-i ''` are not supported. ## New extensions * Unicode characters can be specified in regular expression pattern, replacement diff --git a/src/sed/mod.rs b/src/sed/mod.rs index a4b80260..b03362a5 100644 --- a/src/sed/mod.rs +++ b/src/sed/mod.rs @@ -28,6 +28,7 @@ use crate::sed::script_line_provider::ScriptValue; use clap::{Arg, ArgMatches, Command, arg}; use std::collections::HashMap; use std::env; +use std::ffi::OsString; use std::path::PathBuf; use uucore::error::{UResult, USimpleError, UUsageError}; use uucore::format_usage; @@ -38,7 +39,7 @@ const VERSION: &str = concat!(env!("CARGO_PKG_VERSION"), " (uutils)"); #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let matches = uu_app().try_get_matches_from(args)?; + let matches = parse_args(args)?; // Don't use arg_required_else_help when declaring command // as it exits with code 2 and we use it to check @@ -56,6 +57,66 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { Ok(()) } +fn parse_args(args: impl IntoIterator) -> clap::error::Result { + let mut cmd = uu_app(); + cmd.build(); + let args = gnu_in_place_args(&cmd, args); + cmd.try_get_matches_from(args) +} + +/// Rewrite GNU's `-iSUFFIX`, which clap cannot parse, as `--in-place=SUFFIX`, +/// splitting it from flags clustered before it: `-ni.bak` becomes +/// `-n --in-place=.bak`. +fn gnu_in_place_args(cmd: &Command, args: impl IntoIterator) -> Vec { + let mut args = args.into_iter(); + // The program name. + let mut out: Vec = args.next().into_iter().collect(); + for arg in args.by_ref() { + if arg == "--" { + out.push(arg); + break; + } + // Pass non-UTF-8 arguments to clap unchanged. + let Some((flags, suffix)) = arg.to_str().and_then(|s| { + let pos = in_place_option_position(cmd, s)?; + Some((&s[..pos], &s[pos + 1..])) + }) else { + out.push(arg); + continue; + }; + if flags != "-" { + out.push(flags.into()); + } + out.push(format!("--in-place={suffix}").into()); + } + out.extend(args); + out +} + +/// Return the position of the `i` in a cluster like `-ni.bak`, if a suffix +/// follows it. +fn in_place_option_position(cmd: &Command, arg: &str) -> Option { + let cluster = arg + .as_bytes() + .strip_prefix(b"-") + .filter(|c| !c.starts_with(b"-"))?; + for (pos, &byte) in cluster.iter().enumerate() { + // All short options are ASCII. Leave unknown options for clap to report. + let c = char::from(byte); + let opt = cmd.get_arguments().find(|a| { + a.get_short() == Some(c) || a.get_all_short_aliases().is_some_and(|s| s.contains(&c)) + })?; + if opt.get_id() == "in-place" { + return (pos + 1 < cluster.len()).then_some(pos + 1); + } + if opt.get_action().takes_values() { + // The rest of the cluster is this option's value. + return None; + } + } + None +} + #[allow(clippy::cognitive_complexity)] pub fn uu_app() -> Command { #[cfg(windows)] @@ -101,11 +162,14 @@ pub fn uu_app() -> Command { .help("Follow symlinks when processing in place.") .action(clap::ArgAction::SetTrue), // Access with .get_one::("in-place") + // The SUFFIX must be attached, as in GNU sed. Arg::new("in-place") .short('i') .long("in-place") .help("Edit files in place, making a backup if SUFFIX is supplied.") + .value_name("SUFFIX") .num_args(0..=1) + .require_equals(true) .default_missing_value(""), // Access with .get_one::("line-length") arg!(-l --length "Specify the 'l' command line-wrap length.") @@ -445,13 +509,124 @@ mod tests { assert!(ctx.regex_extended); } + // In-place argument rewriting + fn in_place_args(args: &[&str]) -> Vec { + let mut cmd = uu_app(); + cmd.build(); + gnu_in_place_args(&cmd, ["sed"].iter().chain(args).map(OsString::from)) + .into_iter() + .skip(1) + .map(|arg| arg.into_string().unwrap()) + .collect() + } + + fn in_place_matches(args: &[&str]) -> ArgMatches { + parse_args(["sed"].iter().chain(args).map(OsString::from)).unwrap() + } + #[test] - fn test_in_place_with_suffix() { - let matches = test_matches(&["-i", ".bak"]); - let ctx = build_context(&matches).unwrap(); + fn test_in_place_args_rewrite_attached_suffix() { + let cases: &[(&[&str], &[&str])] = &[ + (&["-i.bak"], &["--in-place=.bak"]), + // Everything after the `i` is the suffix, as in GNU sed. + (&["-iE"], &["--in-place=E"]), + (&["-i=.bak"], &["--in-place==.bak"]), + // Flags clustered before `-i` are kept. + (&["-ni.bak"], &["-n", "--in-place=.bak"]), + (&["-Esi.bak"], &["-Es", "--in-place=.bak"]), + (&["-ri.bak"], &["-r", "--in-place=.bak"]), + ( + &["-i.bak", "--", "-i.keep"], + &["--in-place=.bak", "--", "-i.keep"], + ), + ]; + for (args, expected) in cases { + assert_eq!(in_place_args(args), *expected, "args: {args:?}"); + } + } - assert!(ctx.in_place); - assert_eq!(ctx.in_place_suffix, Some(".bak".to_string())); + #[test] + fn test_in_place_args_leave_other_arguments_alone() { + let cases: &[&[&str]] = &[ + &["-i", "s/a/b/", "file"], + &["-Ei", "s/a/b/", "file"], + &["-En", "s/a/b/", "file"], + &["--in-place=.bak", "s/a/b/", "file"], + &["--in=.bak", "s/a/b/", "file"], + // Values attached to other options. + &["-fi.sed", "file"], + &["-nfi.sed", "file"], + &["-ei", "file"], + // Unknown options are left for clap to report. + &["-xi.bak"], + &["--", "-i.bak"], + ]; + for args in cases { + assert_eq!(in_place_args(args), *args, "args: {args:?}"); + } + } + + #[test] + fn test_in_place_suffix_forms() { + let cases: &[(&[&str], Option<&str>)] = &[ + (&["-i"], None), + (&["--in-place"], None), + (&["-i.bak"], Some(".bak")), + (&["--in-place=.bak"], Some(".bak")), + (&["-i=.bak"], Some("=.bak")), + (&["-iE"], Some("E")), + ]; + for (args, suffix) in cases { + let args: Vec<&str> = args.iter().copied().chain(["s/a/b/", "file"]).collect(); + let matches = in_place_matches(&args); + let ctx = build_context(&matches).unwrap(); + let (scripts, files) = get_scripts_files(&matches).unwrap(); + + assert!(ctx.in_place, "args: {args:?}"); + assert_eq!(ctx.in_place_suffix.as_deref(), *suffix, "args: {args:?}"); + assert!(!ctx.regex_extended, "args: {args:?}"); + assert_eq!( + scripts, + vec![ScriptValue::StringVal("s/a/b/".to_string())], + "args: {args:?}" + ); + assert_eq!(files, vec![PathBuf::from("file")], "args: {args:?}"); + } + } + + #[test] + fn test_in_place_clustered_with_flags() { + for (arg, suffix) in [("-nEi", None), ("-nEi.bak", Some(".bak"))] { + let matches = in_place_matches(&[arg, "s/a/b/p", "file"]); + let ctx = build_context(&matches).unwrap(); + let (scripts, files) = get_scripts_files(&matches).unwrap(); + + assert!(ctx.quiet, "{arg}"); + assert!(ctx.regex_extended, "{arg}"); + assert!(ctx.in_place, "{arg}"); + assert_eq!(ctx.in_place_suffix.as_deref(), suffix, "{arg}"); + assert_eq!( + scripts, + vec![ScriptValue::StringVal("s/a/b/p".to_string())], + "{arg}" + ); + assert_eq!(files, vec![PathBuf::from("file")], "{arg}"); + } + } + + #[test] + fn test_in_place_detached_argument_is_not_suffix() { + // BSD sed reads the argument after `-i` as the suffix; GNU sed does not. + for suffix in ["", ".bak"] { + let matches = in_place_matches(&["-i", suffix, "-e", "s/a/b/", "file"]); + let ctx = build_context(&matches).unwrap(); + let (scripts, files) = get_scripts_files(&matches).unwrap(); + + assert!(ctx.in_place); + assert_eq!(ctx.in_place_suffix, None); + assert_eq!(scripts, vec![ScriptValue::StringVal("s/a/b/".to_string())]); + assert_eq!(files, vec![PathBuf::from(suffix), PathBuf::from("file")]); + } } #[test] diff --git a/tests/by-util/test_sed.rs b/tests/by-util/test_sed.rs index ace171d3..2a208f55 100644 --- a/tests/by-util/test_sed.rs +++ b/tests/by-util/test_sed.rs @@ -2255,6 +2255,47 @@ fn in_place_edit_replace() -> std::io::Result<()> { Ok(()) } +// `sed -i SCRIPT FILE`: the argument after `-i` is the script, not a suffix. +#[test] +fn in_place_edit_script_after_bare_i() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("input"); + + std::fs::write(&path, "hello, world\n")?; + + new_ucmd!() + .args(&["-i", "s/world/universe/", path.to_str().unwrap()]) + .succeeds(); + + let actual = std::fs::read_to_string(&path)?; + assert_eq!(actual, "hello, universe\n"); + Ok(()) +} + +// As in GNU sed, BSD's detached suffix is a script or an input file. +#[test] +fn in_place_edit_detached_suffix_is_not_suffix() -> std::io::Result<()> { + let script = "s/world/universe/"; + let cases: &[(&[&str], &str)] = &[ + (&["-i", "", "-e", script], "''"), + (&["-i", ".bak", "-e", script], "'.bak'"), + // The empty argument is the script, and the script is an input file. + (&["-i", "", script], "'s/world/universe/'"), + ]; + for (args, missing) in cases { + let dir = tempfile::tempdir()?; + let path = dir.path().join("input"); + std::fs::write(&path, "hello, world\n")?; + + new_ucmd!() + .args(args) + .arg(&path) + .fails() + .stderr_contains(format!("error opening input file {missing}")); + } + Ok(()) +} + #[test] fn in_place_edit_backup() -> std::io::Result<()> { let dir = tempfile::tempdir()?; @@ -2263,13 +2304,7 @@ fn in_place_edit_backup() -> std::io::Result<()> { std::fs::write(&path, b"hello, world\n")?; new_ucmd!() - .args(&[ - "-i", - ".bak", - "-e", - "s/world/universe/", - path.to_str().unwrap(), - ]) + .args(&["-i.bak", "-e", "s/world/universe/", path.to_str().unwrap()]) .succeeds(); // Read edited file @@ -2287,6 +2322,32 @@ fn in_place_edit_backup() -> std::io::Result<()> { Ok(()) } +#[test] +fn in_place_edit_backup_forms() -> std::io::Result<()> { + for arg in ["-i.bak", "--in-place=.bak", "-ni.bak"] { + let dir = tempfile::tempdir()?; + let path = dir.path().join("input"); + std::fs::write(&path, "hello, world\n")?; + + new_ucmd!() + .args(&[arg, "s/world/universe/p", path.to_str().unwrap()]) + .succeeds(); + + let expected = if arg == "-ni.bak" { + "hello, universe\n" + } else { + "hello, universe\nhello, universe\n" + }; + assert_eq!(std::fs::read_to_string(&path)?, expected, "{arg}"); + assert_eq!( + std::fs::read_to_string(dir.path().join("input.bak"))?, + "hello, world\n", + "{arg}" + ); + } + Ok(()) +} + #[cfg(unix)] #[test] fn in_place_edit_follow_symlink_edits_target() -> Result<(), Box> { @@ -2360,8 +2421,7 @@ fn in_place_edit_follow_symlink_with_backup() -> Result<(), Box Result<(), Box Date: Tue, 29 Sep 2026 20:58:37 +0200 Subject: [PATCH 2/2] sed: leave other options' values alone when rewriting -i --- src/sed/mod.rs | 204 ++++++++++++++++++++------------------ tests/by-util/test_sed.rs | 31 ++++-- 2 files changed, 132 insertions(+), 103 deletions(-) diff --git a/src/sed/mod.rs b/src/sed/mod.rs index b03362a5..868b2bbe 100644 --- a/src/sed/mod.rs +++ b/src/sed/mod.rs @@ -66,55 +66,109 @@ fn parse_args(args: impl IntoIterator) -> clap::error::Result) -> Vec { let mut args = args.into_iter(); // The program name. let mut out: Vec = args.next().into_iter().collect(); - for arg in args.by_ref() { + while let Some(arg) = args.next() { + // Pass non-UTF-8 arguments to clap unchanged. + let arg = match arg.into_string() { + Ok(arg) => arg, + Err(arg) => { + out.push(arg); + continue; + } + }; if arg == "--" { - out.push(arg); + out.push(arg.into()); break; } - // Pass non-UTF-8 arguments to clap unchanged. - let Some((flags, suffix)) = arg.to_str().and_then(|s| { - let pos = in_place_option_position(cmd, s)?; - Some((&s[..pos], &s[pos + 1..])) - }) else { - out.push(arg); - continue; - }; - if flags != "-" { - out.push(flags.into()); + match option_kind(cmd, &arg) { + OptionKind::InPlace(pos) => { + if pos > 1 { + out.push(arg[..pos].into()); + } + out.push(format!("--in-place={}", &arg[pos + 1..]).into()); + } + OptionKind::ValueFollows => { + out.push(arg.into()); + // Keep the value as is, even if it looks like `-iSUFFIX`. + out.extend(args.next()); + } + OptionKind::Other => out.push(arg.into()), } - out.push(format!("--in-place={suffix}").into()); } out.extend(args); out } -/// Return the position of the `i` in a cluster like `-ni.bak`, if a suffix -/// follows it. -fn in_place_option_position(cmd: &Command, arg: &str) -> Option { - let cluster = arg - .as_bytes() - .strip_prefix(b"-") - .filter(|c| !c.starts_with(b"-"))?; - for (pos, &byte) in cluster.iter().enumerate() { - // All short options are ASCII. Leave unknown options for clap to report. - let c = char::from(byte); - let opt = cmd.get_arguments().find(|a| { +enum OptionKind { + /// A cluster like `-ni.bak`, with the position of its `i`. + InPlace(usize), + /// An option whose value is the next argument, like `-f FILE`. + ValueFollows, + Other, +} + +fn option_kind(cmd: &Command, arg: &str) -> OptionKind { + if let Some(name) = arg.strip_prefix("--") { + return match long_option(cmd, name) { + Some(opt) if opt.get_action().takes_values() && !opt.is_require_equals_set() => { + OptionKind::ValueFollows + } + _ => OptionKind::Other, + }; + } + let Some(cluster) = arg.strip_prefix('-') else { + return OptionKind::Other; + }; + for (pos, c) in cluster.char_indices() { + // Leave unknown options for clap to report. + let Some(opt) = cmd.get_arguments().find(|a| { a.get_short() == Some(c) || a.get_all_short_aliases().is_some_and(|s| s.contains(&c)) - })?; + }) else { + return OptionKind::Other; + }; + let attached = pos + c.len_utf8() < cluster.len(); if opt.get_id() == "in-place" { - return (pos + 1 < cluster.len()).then_some(pos + 1); + return if attached { + OptionKind::InPlace(pos + 1) + } else { + OptionKind::Other + }; } if opt.get_action().takes_values() { - // The rest of the cluster is this option's value. - return None; + // The rest of the cluster, or else the next argument, is the value. + return if attached { + OptionKind::Other + } else { + OptionKind::ValueFollows + }; } } - None + OptionKind::Other +} + +/// Find a long option by its name or, as `infer_long_args` allows, a unique +/// prefix of it. `None` if `name` includes a value. +fn long_option<'a>(cmd: &'a Command, name: &str) -> Option<&'a Arg> { + if name.contains('=') { + return None; + } + let names = |a: &'a Arg| { + a.get_long() + .into_iter() + .chain(a.get_all_aliases().into_iter().flatten()) + }; + if let Some(opt) = cmd.get_arguments().find(|a| names(a).any(|n| n == name)) { + return Some(opt); + } + let mut found = cmd + .get_arguments() + .filter(|a| names(a).any(|n| n.starts_with(name))); + let opt = found.next()?; + found.next().is_none().then_some(opt) } #[allow(clippy::cognitive_complexity)] @@ -148,7 +202,9 @@ pub fn uu_app() -> Command { .short_alias('r') .help("Use extended regular expressions.") .action(clap::ArgAction::SetTrue), + // As in GNU sed, a value may begin with `-`. arg!(-e --expression