sed: take the -i suffix only when attached, as GNU sed does

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

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,120 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     Ok(())
 }
 
+fn parse_args(args: impl IntoIterator<Item = OsString>) -> clap::error::Result<ArgMatches> {
+    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`. The values of other options are left as they are.
+fn gnu_in_place_args(cmd: &Command, args: impl IntoIterator<Item = OsString>) -> Vec<OsString> {
+    let mut args = args.into_iter();
+    // The program name.
+    let mut out: Vec<OsString> = args.next().into_iter().collect();
+    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.into());
+            break;
+        }
+        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.extend(args);
+    out
+}
+
+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 if attached {
+                OptionKind::InPlace(pos + 1)
+            } else {
+                OptionKind::Other
+            };
+        }
+        if opt.get_action().takes_values() {
+            // The rest of the cluster, or else the next argument, is the value.
+            return if attached {
+                OptionKind::Other
+            } else {
+                OptionKind::ValueFollows
+            };
+        }
+    }
+    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)]
 pub fn uu_app() -> Command {
     #[cfg(windows)]
@@ -87,28 +202,35 @@ 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 <SCRIPT> "Add script to executed commands.")
+                .allow_hyphen_values(true)
                 .action(clap::ArgAction::Append),
             // Access with .get_many::<PathBuf>("file")
             Arg::new("script-file")
                 .short('f')
                 .long("script-file")
                 .help("Specify script file.")
                 .value_parser(clap::value_parser!(PathBuf))
+                .allow_hyphen_values(true)
                 .action(clap::ArgAction::Append),
             Arg::new("follow-symlinks")
                 .long("follow-symlinks")
                 .help("Follow symlinks when processing in place.")
                 .action(clap::ArgAction::SetTrue),
             // Access with .get_one::<String>("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::<u32>("line-length")
             arg!(-l --length <NUM> "Specify the 'l' command line-wrap length.")
+                .allow_hyphen_values(true)
                 .value_parser(clap::value_parser!(u32)),
             arg!(-n --quiet "Suppress automatic printing of pattern space.").aliases(["silent"]),
             arg!(--posix "Disable non-POSIX extensions."),
@@ -445,13 +567,76 @@ mod tests {
         assert!(ctx.regex_extended);
     }
 
+    // In-place argument rewriting
+    fn in_place_args(args: &[&str]) -> Vec<String> {
+        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()
+    }
+
     #[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"],
+            ),
+            // Only the next argument is another option's value.
+            (&["-e", "p", "-i.bak"], &["-e", "p", "--in-place=.bak"]),
+            (
+                &["--expression=p", "-i.bak"],
+                &["--expression=p", "--in-place=.bak"],
+            ),
+            (&["--quiet", "-i.bak"], &["--quiet", "--in-place=.bak"]),
+            (
+                &["--in-place", "-i.bak"],
+                &["--in-place", "--in-place=.bak"],
+            ),
+            // A `--` that is an option's value does not end the options.
+            (&["-e", "--", "-i.bak"], &["-e", "--", "--in-place=.bak"]),
+        ];
+        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"],
+            // Values that follow other options.
+            &["-f", "-ifoo.sed", "file"],
+            &["-nf", "-ifoo.sed", "file"],
+            &["-l", "-i5"],
+            &["--expression", "-i.bak"],
+            &["--expr", "-i.bak"],
+            // 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]

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,51 @@ fn in_place_edit_backup() -> std::io::Result<()> {
     Ok(())
 }
 
+#[test]
+fn in_place_edit_backup_forms() -> std::io::Result<()> {
+    for (arg, expected) in [
+        ("-i.bak", "hello, universe\nhello, universe\n"),
+        ("--in-place=.bak", "hello, universe\nhello, universe\n"),
+        ("-ni.bak", "hello, universe\n"),
+    ] {
+        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();
+
+        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(())
+}
+
+// As in GNU sed, the value of `-f` is not taken as `-i`.
+#[test]
+fn script_file_named_like_in_place_option() -> std::io::Result<()> {
+    let dir = tempfile::tempdir()?;
+    std::fs::write(dir.path().join("-ifoo.sed"), "s/world/universe/\n")?;
+    std::fs::write(dir.path().join("input"), "hello, world\n")?;
+
+    new_ucmd!()
+        .current_dir(dir.path())
+        .args(&["-f", "-ifoo.sed", "input"])
+        .succeeds()
+        .stdout_is("hello, universe\n");
+
+    assert_eq!(
+        std::fs::read_to_string(dir.path().join("input"))?,
+        "hello, world\n"
+    );
+    Ok(())
+}
+
 #[cfg(unix)]
 #[test]
 fn in_place_edit_follow_symlink_edits_target() -> Result<(), Box<dyn std::error::Error>> {
@@ -2360,8 +2440,7 @@ fn in_place_edit_follow_symlink_with_backup() -> Result<(), Box<dyn std::error::
     new_ucmd!()
         .args(&[
             "--follow-symlinks",
-            "-i",
-            ".bak",
+            "-i.bak",
             "-e",
             "s/world/universe/",
             link.path().to_str().unwrap(),
@@ -2396,8 +2475,7 @@ fn in_place_edit_symlink_replaced_with_backup() -> Result<(), Box<dyn std::error
 
     new_ucmd!()
         .args(&[
-            "-i",
-            ".bak",
+            "-i.bak",
             "-e",
             "s/world/universe/",
             link.path().to_str().unwrap(),