From a86b08ab5f1b602052d20e35370ef0f6d379d9cc Mon Sep 17 00:00:00 2001 From: mhead Date: Wed, 29 May 2024 17:57:19 +0530 Subject: [PATCH 1/5] cp: attrs overriding --- src/uu/cp/src/cp.rs | 167 +++++++++++++++++++++++++++++++++----------- 1 file changed, 125 insertions(+), 42 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 49e85265599..75776dae9b0 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -7,7 +7,6 @@ use quick_error::quick_error; use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; -use std::env; #[cfg(not(windows))] use std::ffi::CString; use std::fs::{self, File, Metadata, OpenOptions, Permissions}; @@ -406,6 +405,11 @@ static PRESERVABLE_ATTRIBUTES: &[&str] = &[ static PRESERVABLE_ATTRIBUTES: &[&str] = &["mode", "timestamps", "context", "links", "xattr", "all"]; +const PRESERVE_DEFAULT_VALUES: &str = if cfg!(unix) { + "mode,ownership,timestamp" +} else { + "mode,timestamp" +}; pub fn uu_app() -> Command { const MODE_ARGS: &[&str] = &[ options::LINK, @@ -556,11 +560,7 @@ pub fn uu_app() -> Command { .num_args(0..) .require_equals(true) .value_name("ATTR_LIST") - .overrides_with_all([ - options::ARCHIVE, - options::PRESERVE_DEFAULT_ATTRIBUTES, - options::NO_PRESERVE, - ]) + .default_missing_value(PRESERVE_DEFAULT_VALUES) // -d sets this option // --archive sets this option .help( @@ -572,19 +572,18 @@ pub fn uu_app() -> Command { Arg::new(options::PRESERVE_DEFAULT_ATTRIBUTES) .short('p') .long(options::PRESERVE_DEFAULT_ATTRIBUTES) - .overrides_with_all([options::PRESERVE, options::NO_PRESERVE, options::ARCHIVE]) .help("same as --preserve=mode,ownership(unix only),timestamps") .action(ArgAction::SetTrue), ) .arg( Arg::new(options::NO_PRESERVE) .long(options::NO_PRESERVE) + .action(ArgAction::Append) + .use_value_delimiter(true) + .value_parser(ShortcutValueParser::new(PRESERVABLE_ATTRIBUTES)) + .num_args(0..) + .require_equals(true) .value_name("ATTR_LIST") - .overrides_with_all([ - options::PRESERVE_DEFAULT_ATTRIBUTES, - options::PRESERVE, - options::ARCHIVE, - ]) .help("don't preserve the specified attributes"), ) .arg( @@ -621,11 +620,6 @@ pub fn uu_app() -> Command { Arg::new(options::ARCHIVE) .short('a') .long(options::ARCHIVE) - .overrides_with_all([ - options::PRESERVE_DEFAULT_ATTRIBUTES, - options::PRESERVE, - options::NO_PRESERVE, - ]) .help("Same as -dR --preserve=all") .action(ArgAction::SetTrue), ) @@ -839,6 +833,44 @@ impl Attributes { } } + /// Set the field to Preserve::NO { explicit: true } if the corresponding field + /// in other is set to Preserve::Yes { .. }. + pub fn diff(self, other: &Self) -> Self { + Self { + #[cfg(unix)] + ownership: if matches!(other.ownership, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.ownership + }, + mode: if matches!(other.mode, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.mode + }, + timestamps: if matches!(other.timestamps, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.timestamps + }, + context: if matches!(other.context, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.context + }, + links: if matches!(other.links, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.links + }, + xattr: if matches!(other.xattr, Preserve::Yes { .. }) { + Preserve::No { explicit: true } + } else { + self.xattr + }, + } + } + pub fn parse_iter(values: impl Iterator) -> Result where T: AsRef, @@ -926,34 +958,85 @@ impl Options { } }; - // Parse attributes to preserve - let mut attributes = - if let Some(attribute_strs) = matches.get_many::(options::PRESERVE) { - if attribute_strs.len() == 0 { - Attributes::DEFAULT - } else { - Attributes::parse_iter(attribute_strs)? + // 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. + 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 + // `overriding_order`. + for option in [ + options::PRESERVE, + options::NO_PRESERVE, + options::ARCHIVE, + options::PRESERVE_DEFAULT_ATTRIBUTES, + options::NO_DEREFERENCE_PRESERVE_LINKS, + ] { + if let (Ok(Some(val)), Some(index)) = ( + matches.try_get_one::(option), + // even though it says in the doc that `index_of` would give us + // the first index of the argument, when it comes to flag it + // gives us the last index where the flag appeared (probably + // because it overrides itself). Since it is a flag and it would + // have same value across the occurrences we just need the last + // index. + matches.index_of(option), + ) { + if *val { + overriding_order.push((index, option, vec![])); } - } else if matches.get_flag(options::ARCHIVE) { - // --archive is used. Same as --preserve=all - Attributes::ALL - } else if matches.get_flag(options::NO_DEREFERENCE_PRESERVE_LINKS) { - Attributes::LINKS - } else if matches.get_flag(options::PRESERVE_DEFAULT_ATTRIBUTES) { - Attributes::DEFAULT - } else { - Attributes::NONE - }; + } else if let (Some(occurrences), Some(mut indices)) = ( + matches.get_occurrences::(option), + matches.indices_of(option), + ) { + occurrences.for_each(|val| { + if let Some(index) = indices.next() { + let val = val.collect::>(); + // As mentioned in the documentation of the indices_of + // function, it provides the indices of the individual + // values. Therefore, to get the index of the first + // value of the next occurrence in the next iteration, + // we need to advance the indices iterator by the length + // of the current occurrence's values. + for _ in 1..val.len() { + indices.next(); + } + overriding_order.push((index, option, val)); + } + }); + } + } + overriding_order.sort_by(|a, b| a.0.cmp(&b.0)); - // handling no-preserve options and adjusting the attributes - if let Some(attribute_strs) = matches.get_many::(options::NO_PRESERVE) { - if attribute_strs.len() > 0 { - let no_preserve_attributes = Attributes::parse_iter(attribute_strs)?; - if matches!(no_preserve_attributes.links, Preserve::Yes { .. }) { - attributes.links = Preserve::No { explicit: true }; - } else if matches!(no_preserve_attributes.mode, Preserve::Yes { .. }) { - attributes.mode = Preserve::No { explicit: true }; + let mut attributes = Attributes::NONE; + + // Iterate through the `overriding_order` and adjust the attributes accordingly. + for (_, option, val) in overriding_order { + match option { + options::ARCHIVE => { + attributes = Attributes::ALL; + } + options::PRESERVE_DEFAULT_ATTRIBUTES => { + attributes = attributes.union(&Attributes::DEFAULT); + } + options::NO_DEREFERENCE_PRESERVE_LINKS => { + attributes = attributes.union(&Attributes::LINKS); + } + options::PRESERVE => { + attributes = attributes.union(&Attributes::parse_iter(val.into_iter())?); + } + options::NO_PRESERVE => { + if !val.is_empty() { + attributes = attributes.diff(&Attributes::parse_iter(val.into_iter())?); + } } + _ => (), } } From 52481b79f3c9cc944510e5115ab18473b7862434 Mon Sep 17 00:00:00 2001 From: mhead Date: Wed, 29 May 2024 23:25:20 +0530 Subject: [PATCH 2/5] cp: attrs overriding tests --- tests/by-util/test_cp.rs | 71 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index a9032f215e9..1e6586dc830 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -5502,3 +5502,74 @@ fn test_dir_perm_race_with_preserve_mode_and_ownership() { child.wait().unwrap().succeeded(); } } + +#[test] +// when -d and -a are overridden with --preserve or --no-preserve make sure that it only +// overrides attributes not other flags like -r or --no_deref implied in -a and -d. +fn test_preserve_attrs_overriding_1() { + const FILE: &str = "file"; + const SYMLINK: &str = "symlink"; + const DEST: &str = "dest"; + for f in ["-d", "-a"] { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.make_file(FILE); + at.symlink_file(FILE, SYMLINK); + scene + .ucmd() + .args(&[f, "--no-preserve=all", SYMLINK, DEST]) + .succeeds(); + at.symlink_exists(DEST); + } +} + +#[test] +#[cfg(all(unix, not(target_os = "android")))] +fn test_preserve_attrs_overriding_2() { + const FILE1: &str = "file1"; + const FILE2: &str = "file2"; + const FOLDER: &str = "folder"; + const DEST: &str = "dest"; + for mut args in [ + // All of the following to args should tell cp to preserve mode and + // timestamp, but not the link. + vec!["-r", "--preserve=mode,link,timestamp", "--no-preserve=link"], + vec![ + "-r", + "--preserve=mode", + "--preserve=link", + "--preserve=timestamp", + "--no-preserve=link", + ], + vec![ + "-r", + "--preserve=mode,link", + "--no-preserve=link", + "--preserve=timestamp", + ], + vec!["-a", "--no-preserve=link"], + vec!["-r", "--preserve", "--no-preserve=link"], + ] { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir(FOLDER); + at.make_file(&format!("{}/{}", FOLDER, FILE1)); + at.set_mode(&format!("{}/{}", FOLDER, FILE1), 0o775); + at.hard_link( + &format!("{}/{}", FOLDER, FILE1), + &format!("{}/{}", FOLDER, FILE2), + ); + args.append(&mut vec![FOLDER, DEST]); + let src_file1_metadata = at.metadata(&format!("{}/{}", FOLDER, FILE1)); + scene.ucmd().args(&args).succeeds(); + at.dir_exists(DEST); + let dest_file1_metadata = at.metadata(&format!("{}/{}", DEST, FILE1)); + let dest_file2_metadata = at.metadata(&format!("{}/{}", DEST, FILE2)); + assert_eq!( + src_file1_metadata.modified().unwrap(), + dest_file1_metadata.modified().unwrap() + ); + assert_eq!(src_file1_metadata.mode(), dest_file1_metadata.mode()); + assert_ne!(dest_file1_metadata.ino(), dest_file2_metadata.ino()); + } +} From e0a34b557d64a5e8e08c00dda8aeec60a3aa3f03 Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 30 May 2024 12:44:03 +0530 Subject: [PATCH 3/5] cp: attrs overriding unit tests --- src/uu/cp/src/cp.rs | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 75776dae9b0..5492d1f3dd6 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -169,7 +169,7 @@ pub enum CopyMode { /// For full compatibility with GNU, these options should also combine. We /// currently only do a best effort imitation of that behavior, because it is /// difficult to achieve in clap, especially with `--no-preserve`. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub struct Attributes { #[cfg(unix)] pub ownership: Preserve, @@ -2390,7 +2390,7 @@ fn disk_usage_directory(p: &Path) -> io::Result { #[cfg(test)] mod tests { - use crate::{aligned_ancestors, localize_to_target}; + use crate::{aligned_ancestors, localize_to_target, Attributes, Preserve}; use std::path::Path; #[test] @@ -2412,4 +2412,36 @@ mod tests { ]; assert_eq!(actual, expected); } + #[test] + fn test_diff_attrs() { + assert_eq!( + Attributes::ALL.diff(&Attributes { + context: Preserve::Yes { required: true }, + xattr: Preserve::Yes { required: true }, + ..Attributes::ALL + }), + Attributes { + #[cfg(unix)] + ownership: Preserve::No { explicit: true }, + mode: Preserve::No { explicit: true }, + timestamps: Preserve::No { explicit: true }, + context: Preserve::No { explicit: true }, + links: Preserve::No { explicit: true }, + xattr: Preserve::No { explicit: true } + } + ); + assert_eq!( + Attributes { + context: Preserve::Yes { required: true }, + xattr: Preserve::Yes { required: true }, + ..Attributes::ALL + } + .diff(&Attributes::NONE), + Attributes { + context: Preserve::Yes { required: true }, + xattr: Preserve::Yes { required: true }, + ..Attributes::ALL + } + ); + } } From 16c6c6e632cb7569b1fb4afba6b61ea4f5cd8430 Mon Sep 17 00:00:00 2001 From: sreehari prasad <52113972+matrixhead@users.noreply.github.com> Date: Thu, 30 May 2024 14:08:22 +0530 Subject: [PATCH 4/5] Update src/uu/cp/src/cp.rs Co-authored-by: Sylvestre Ledru --- src/uu/cp/src/cp.rs | 45 +++++++++++++++------------------------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 5492d1f3dd6..590b8fd158b 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -835,39 +835,24 @@ impl Attributes { /// Set the field to Preserve::NO { explicit: true } if the corresponding field /// in other is set to Preserve::Yes { .. }. - pub fn diff(self, other: &Self) -> Self { Self { - #[cfg(unix)] - ownership: if matches!(other.ownership, Preserve::Yes { .. }) { - Preserve::No { explicit: true } - } else { - self.ownership - }, - mode: if matches!(other.mode, Preserve::Yes { .. }) { - Preserve::No { explicit: true } - } else { - self.mode - }, - timestamps: if matches!(other.timestamps, Preserve::Yes { .. }) { - Preserve::No { explicit: true } - } else { - self.timestamps - }, - context: if matches!(other.context, Preserve::Yes { .. }) { - Preserve::No { explicit: true } - } else { - self.context - }, - links: if matches!(other.links, Preserve::Yes { .. }) { - Preserve::No { explicit: true } - } else { - self.links - }, - xattr: if matches!(other.xattr, Preserve::Yes { .. }) { + pub fn diff(self, other: &Self) -> Self { + fn update_preserve_field(current: Preserve, other: Preserve) -> Preserve { + if matches!(other, Preserve::Yes { .. }) { Preserve::No { explicit: true } } else { - self.xattr - }, + current + } + } + + Self { + #[cfg(unix)] + ownership: update_preserve_field(self.ownership, other.ownership), + mode: update_preserve_field(self.mode, other.mode), + timestamps: update_preserve_field(self.timestamps, other.timestamps), + context: update_preserve_field(self.context, other.context), + links: update_preserve_field(self.links, other.links), + xattr: update_preserve_field(self.xattr, other.xattr), } } From 849cf4877966d2f881fab78d2fa74e1b341e7d24 Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 30 May 2024 16:48:25 +0530 Subject: [PATCH 5/5] cp: attrs overriding: minor changes --- src/uu/cp/src/cp.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 590b8fd158b..2b2a2a2bf09 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -835,7 +835,6 @@ impl Attributes { /// Set the field to Preserve::NO { explicit: true } if the corresponding field /// in other is set to Preserve::Yes { .. }. - Self { pub fn diff(self, other: &Self) -> Self { fn update_preserve_field(current: Preserve, other: Preserve) -> Preserve { if matches!(other, Preserve::Yes { .. }) { @@ -844,7 +843,6 @@ impl Attributes { current } } - Self { #[cfg(unix)] ownership: update_preserve_field(self.ownership, other.ownership), @@ -2428,5 +2426,21 @@ mod tests { ..Attributes::ALL } ); + assert_eq!( + Attributes::NONE.diff(&Attributes { + context: Preserve::Yes { required: true }, + xattr: Preserve::Yes { required: true }, + ..Attributes::ALL + }), + Attributes { + #[cfg(unix)] + ownership: Preserve::No { explicit: true }, + mode: Preserve::No { explicit: true }, + timestamps: Preserve::No { explicit: true }, + context: Preserve::No { explicit: true }, + links: Preserve::No { explicit: true }, + xattr: Preserve::No { explicit: true } + } + ); } }