From c83b92bdd4c3445827c783de0855c382cf0aff39 Mon Sep 17 00:00:00 2001 From: Simon Johnsson Date: Thu, 2 Jul 2026 16:43:02 +0200 Subject: [PATCH] fix(cp): remove overrides_with_all from flag group The -a/-L/-H/-P/-d flags form a mutual-exclusion group for symlink handling. Previously, claps overrides_with_all handled precedence by stripping earlier flags. However, -a is a composite flag (-dR --preserve=all), and stripping it wholesale loses recursive and --preserve=all. Fix: remove overrides_with_all from all five flags. All coexist in the clap match. Precedence is now handled in overriding_order: - Add DEREFERENCE, NO_DEREFERENCE, CLI_SYMBOLIC_LINKS to the order - Derive both dereference and cli_dereference from the last flag in the mutual-exclusion group Fixes: #13207 Also fixes: - -dL: no longer loses Attributes::LINKS - -aH: dereference correctly respects last-flag-wins --- src/uu/cp/src/cp.rs | 114 +++++++++++++++++------------- tests/by-util/test_cp.rs | 146 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 212 insertions(+), 48 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index ccf414beb5..e2eda8c7e4 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -696,12 +696,6 @@ pub fn uu_app() -> Command { Arg::new(options::NO_DEREFERENCE) .short('P') .long(options::NO_DEREFERENCE) - .overrides_with_all([ - options::DEREFERENCE, - options::CLI_SYMBOLIC_LINKS, - options::ARCHIVE, - options::NO_DEREFERENCE_PRESERVE_LINKS, - ]) // -d sets this option .help(translate!("cp-help-no-dereference")) .action(ArgAction::SetTrue), @@ -710,24 +704,12 @@ pub fn uu_app() -> Command { Arg::new(options::DEREFERENCE) .short('L') .long(options::DEREFERENCE) - .overrides_with_all([ - options::NO_DEREFERENCE, - options::CLI_SYMBOLIC_LINKS, - options::ARCHIVE, - options::NO_DEREFERENCE_PRESERVE_LINKS, - ]) .help(translate!("cp-help-dereference")) .action(ArgAction::SetTrue), ) .arg( Arg::new(options::CLI_SYMBOLIC_LINKS) .short('H') - .overrides_with_all([ - options::DEREFERENCE, - options::NO_DEREFERENCE, - options::ARCHIVE, - options::NO_DEREFERENCE_PRESERVE_LINKS, - ]) .help(translate!("cp-help-cli-symbolic-links")) .action(ArgAction::SetTrue), ) @@ -735,24 +717,12 @@ pub fn uu_app() -> Command { Arg::new(options::ARCHIVE) .short('a') .long(options::ARCHIVE) - .overrides_with_all([ - options::DEREFERENCE, - options::NO_DEREFERENCE, - options::CLI_SYMBOLIC_LINKS, - options::NO_DEREFERENCE_PRESERVE_LINKS, - ]) .help(translate!("cp-help-archive")) .action(ArgAction::SetTrue), ) .arg( Arg::new(options::NO_DEREFERENCE_PRESERVE_LINKS) .short('d') - .overrides_with_all([ - options::DEREFERENCE, - options::NO_DEREFERENCE, - options::CLI_SYMBOLIC_LINKS, - options::ARCHIVE, - ]) .help(translate!("cp-help-no-dereference-preserve-links")) .action(ArgAction::SetTrue), ) @@ -1028,6 +998,54 @@ impl Attributes { } } +/// Flags in the mutual-exclusion group for symlink handling. +/// The last one on the command line determines dereference behavior. +/// Note: CLI_SYMBOLIC_LINKS (-H) is excluded here — it only affects +/// CLI-level symlinks, not recursive traversal. +const DEREF_FLAGS: &[&str] = &[ + options::DEREFERENCE, + options::NO_DEREFERENCE, + options::ARCHIVE, + options::NO_DEREFERENCE_PRESERVE_LINKS, +]; + +/// Resolves `(dereference, cli_dereference)` from the overriding order. +/// +/// Both values follow last-flag-wins semantics over the mutual-exclusion +/// group of symlink-handling flags (-L/-H/-P/-a/-d). When no dereference +/// flag is present, `dereference` falls back to its default: don't follow +/// links during recursive copies (unless `--link`). +fn resolve_dereference( + recursive: bool, + is_link: bool, + overriding_order: &[(usize, &str, Vec<&String>)], +) -> (bool, bool) { + let last_deref = overriding_order + .iter() + .rev() + .find_map(|(_, opt, _)| DEREF_FLAGS.contains(opt).then_some(*opt)); + + let dereference = match last_deref { + Some(options::DEREFERENCE) => true, + Some(_) => false, + None => !recursive || is_link, + }; + + let cli_dereference = overriding_order + .iter() + .rev() + .find_map(|(_, opt, _)| match *opt { + options::CLI_SYMBOLIC_LINKS | options::DEREFERENCE => Some(true), + options::ARCHIVE | options::NO_DEREFERENCE | options::NO_DEREFERENCE_PRESERVE_LINKS => { + Some(false) + } + _ => None, + }) + .unwrap_or(false); + + (dereference, cli_dereference) +} + impl Options { #[allow(clippy::cognitive_complexity)] fn from_matches(matches: &ArgMatches) -> CopyResult { @@ -1047,6 +1065,8 @@ impl Options { let recursive = matches.get_flag(options::RECURSIVE) || matches.get_flag(options::ARCHIVE); + let copy_mode = CopyMode::from_matches(matches); + let backup_mode = match backup_control::determine_backup_mode(matches) { Err(e) => return Err(CpError::Backup(BackupError(format!("{e}")))), Ok(mode) => mode, @@ -1079,14 +1099,11 @@ impl Options { return Err(CpError::NotADirectory(dir.clone())); } // cp follows POSIX conventions for overriding options such as "-a", - // "-d", "--preserve", and "--no-preserve". We can use clap's - // override-all behavior to achieve this, but there's a challenge: when - // clap overrides an argument, it removes all traces of it from the - // match. This poses a problem because flags like "-a" expand to "-dR - // --preserve=all", and we only want to override the "--preserve=all" - // part. Additionally, we need to handle multiple occurrences of the - // same flags. To address this, we create an overriding order from the - // matches here. + // "-d", "--preserve", and "--no-preserve": the last flag on the + // command line wins. We build an `overriding_order` vector of all + // overriding options (including dereference flags) sorted by their + // command-line index, then derive attributes and dereference behavior + // from the last relevant entry. let mut overriding_order: Vec<(usize, &str, Vec<&String>)> = vec![]; // We iterate through each overriding option, adding each occurrence of // the option along with its value and index as a tuple, and push it to @@ -1097,6 +1114,9 @@ impl Options { options::ARCHIVE, options::PRESERVE_DEFAULT_ATTRIBUTES, options::NO_DEREFERENCE_PRESERVE_LINKS, + options::DEREFERENCE, + options::NO_DEREFERENCE, + options::CLI_SYMBOLIC_LINKS, ] { if let (Ok(Some(val)), Some(index)) = ( matches.try_get_one::(option), @@ -1134,6 +1154,11 @@ impl Options { } overriding_order.sort_by_key(|a| a.0); + // dereference and cli_dereference follow last-flag-wins semantics + // over the mutual-exclusion group of symlink-handling flags. + let (dereference, cli_dereference) = + resolve_dereference(recursive, copy_mode == CopyMode::Link, &overriding_order); + let mut attributes = Attributes::NONE; // Iterate through the `overriding_order` and adjust the attributes accordingly. @@ -1194,16 +1219,9 @@ impl Options { let options = Self { attributes_only: matches.get_flag(options::ATTRIBUTES_ONLY), copy_contents: matches.get_flag(options::COPY_CONTENTS), - cli_dereference: matches.get_flag(options::CLI_SYMBOLIC_LINKS), - copy_mode: CopyMode::from_matches(matches), - // No dereference is set with -p, -d and --archive - dereference: !(matches.get_flag(options::NO_DEREFERENCE) - || matches.get_flag(options::NO_DEREFERENCE_PRESERVE_LINKS) - || matches.get_flag(options::ARCHIVE) - // cp normally follows the link only when not copying recursively or when - // --link (-l) is used - || (recursive && CopyMode::from_matches(matches)!= CopyMode::Link )) - || matches.get_flag(options::DEREFERENCE), + cli_dereference, + copy_mode, + dereference, one_file_system: matches.get_flag(options::ONE_FILE_SYSTEM), parents: matches.get_flag(options::PARENTS), update: update_mode, diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 7261ef51c9..880c70adfa 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7656,6 +7656,152 @@ fn test_cp_archive_deref_flag_ordering() { } } +/// Regression test: -a keeps recursiveness when combined with -L/-H/-d. +/// https://github.com/uutils/coreutils/issues/13207 +#[test] +#[cfg(unix)] +fn test_cp_archive_deref_preserves_recursive() { + for flags in ["-afL", "-aLf", "-aHL", "-adL"] { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("srcdir"); + at.touch("srcdir/file.txt"); + let dest = format!("dest_{}", &flags.replace('-', "")); + ucmd.args(&[flags, "srcdir", &dest]).succeeds(); + assert!( + at.file_exists(format!("{dest}/file.txt")), + "failed for {flags}: destination file missing" + ); + } +} + +/// -aL should preserve file permissions (--preserve=all from -a). +#[test] +#[cfg(unix)] +fn test_cp_archive_deref_preserves_mode() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("srcdir"); + at.touch("srcdir/file.txt"); + at.set_mode("srcdir/file.txt", 0o705); + ucmd.args(&["-aL", "srcdir", "dest"]).succeeds(); + let mode = at.metadata("dest/file.txt").permissions().mode(); + assert_eq!( + mode & 0o777, + 0o705, + "-aL should preserve mode, got 0o{mode:o}" + ); +} + +/// -dL should preserve hardlinks (--preserve=links from -d survives -L override). +#[test] +#[cfg(target_os = "linux")] +fn test_cp_no_deref_preserve_with_deref_keeps_hardlinks() { + use std::os::linux::fs::MetadataExt; + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("file1"); + at.hard_link("file1", "file2"); + at.mkdir("destdir"); + ucmd.args(&["-dL", "file1", "file2", "destdir"]).succeeds(); + // -dL: hardlink preserved → destdir/file1 should have nlink == 2 + // (both file1 and file2 point to the same inode in destdir) + let nlink = at.metadata("destdir/file1").st_nlink(); + assert_eq!( + nlink, 2, + "-dL should preserve hardlinks (expected nlink=2, got nlink={nlink})" + ); +} + +/// -aL inside a directory: inner symlinks should be dereferenced, +/// while -a preserves them (last-flag-wins for dereference). +#[test] +#[cfg(unix)] +fn test_cp_archive_deref_symlinks_inside_dir() { + use std::os::unix::fs::symlink; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("srcdir"); + at.touch("srcdir/real.txt"); + symlink("real.txt", at.plus_as_string("srcdir/link.txt")).unwrap(); + + // -a (no deref): inner symlinks preserved + scene.ucmd().args(&["-a", "srcdir", "dest_a"]).succeeds(); + assert!( + at.is_symlink("dest_a/link.txt"), + "-a: inner symlink should be preserved" + ); + + // -aL (last is -L, deref): inner symlinks dereferenced + scene.ucmd().args(&["-aL", "srcdir", "dest_aL"]).succeeds(); + assert!( + !at.is_symlink("dest_aL/link.txt"), + "-aL: inner symlink should be dereferenced" + ); + + // -La (last is -a, no deref): inner symlinks preserved + scene.ucmd().args(&["-La", "srcdir", "dest_La"]).succeeds(); + assert!( + at.is_symlink("dest_La/link.txt"), + "-La: inner symlink should be preserved" + ); +} + +/// -aH: inner symlinks preserved (a wins for recursive), CLI symlinks followed (H wins for CLI). +#[test] +#[cfg(unix)] +fn test_cp_archive_cli_deref_inner_preserved() { + use std::os::unix::fs::symlink; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("srcdir"); + at.touch("srcdir/real.txt"); + symlink("real.txt", at.plus_as_string("srcdir/link.txt")).unwrap(); + + // -aH: CLI symlink dereferenced, inner symlinks preserved + scene.ucmd().args(&["-aH", "srcdir", "dest_aH"]).succeeds(); + assert!( + at.is_symlink("dest_aH/link.txt"), + "-aH: inner symlink should be preserved (a wins for recursive)" + ); + + // -Ha: CLI + inner symlinks preserved (a wins since last) + scene.ucmd().args(&["-Ha", "srcdir", "dest_Ha"]).succeeds(); + assert!( + at.is_symlink("dest_Ha/link.txt"), + "-Ha: inner symlink should be preserved (a is last)" + ); +} + +/// Precedence: repeating the same flag should take the last position. +#[test] +#[cfg(unix)] +fn test_cp_archive_deref_repeated_flag_last_wins() { + use std::os::unix::fs::symlink; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir("srcdir"); + at.touch("srcdir/real.txt"); + symlink("real.txt", at.plus_as_string("srcdir/link.txt")).unwrap(); + + // -aL -a: -a is last, inner symlinks preserved + scene + .ucmd() + .args(&["-aL", "-a", "srcdir", "dest"]) + .succeeds(); + assert!( + at.is_symlink("dest/link.txt"), + "-aL -a: last -a should preserve inner symlinks" + ); + + // -La -L: -L is last, inner symlinks dereferenced + scene + .ucmd() + .args(&["-La", "-L", "srcdir", "dest2"]) + .succeeds(); + assert!( + !at.is_symlink("dest2/link.txt"), + "-La -L: last -L should dereference inner symlinks" + ); +} + #[test] fn test_cp_circular_symbolic_links_in_directory() { let source_dir = "source_dir";