From 0ba4dad0a660808dbfa3d83860540a3908bb2ff8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jan 2025 18:45:58 +0100 Subject: [PATCH 1/6] print_verbose_ownership_retained_as: siplify the function --- src/uucore/src/lib/features/perms.rs | 32 +++++++++------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 62e7d56ed..3ecac511e 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -457,30 +457,18 @@ impl ChownExecutor { fn print_verbose_ownership_retained_as(&self, path: &Path, uid: u32, gid: Option) { if self.verbosity.level == VerbosityLevel::Verbose { - match (self.dest_uid, self.dest_gid, gid) { - (Some(_), Some(_), Some(gid)) => { - println!( - "ownership of {} retained as {}:{}", - path.quote(), - entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), - entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), - ); - } + let ownership = match (self.dest_uid, self.dest_gid, gid) { + (Some(_), Some(_), Some(gid)) => format!( + "{}:{}", + entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()) + ), (None, Some(_), Some(gid)) => { - println!( - "ownership of {} retained as {}", - path.quote(), - entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), - ); + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()) } - (_, _, _) => { - println!( - "ownership of {} retained as {}", - path.quote(), - entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), - ); - } - } + _ => entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + }; + println!("ownership of {} retained as {}", path.quote(), ownership); } } } From c45bbe3d1c5ff95d7b56fd6b8dd19cd7c9d43aef Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jan 2025 20:48:33 +0100 Subject: [PATCH 2/6] chgrp: add option --from --- src/uu/chgrp/src/chgrp.rs | 24 ++++++- tests/by-util/test_chgrp.rs | 130 ++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index fe5aee872..9fc0cb2ff 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -49,11 +49,27 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { } } }; + + // Handle --from option + let filter = if let Some(from_group) = matches.get_one::(options::FROM) { + match entries::grp2gid(from_group) { + Ok(g) => IfFrom::Group(g), + _ => { + return Err(USimpleError::new( + 1, + format!("invalid group: {}", from_group.quote()), + )) + } + } + } else { + IfFrom::All + }; + Ok(GidUidOwnerFilter { dest_gid, dest_uid: None, raw_owner: raw_group, - filter: IfFrom::All, + filter, }) } @@ -120,6 +136,12 @@ pub fn uu_app() -> Command { .value_hint(clap::ValueHint::FilePath) .help("use RFILE's group rather than specifying GROUP values"), ) + .arg( + Arg::new(options::FROM) + .long(options::FROM) + .value_name("GROUP") + .help("change the group only if its current group matches GROUP"), + ) .arg( Arg::new(options::RECURSIVE) .short('R') diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index eca5ba0ed..086f21cb0 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -417,3 +417,133 @@ fn test_traverse_symlinks() { ); } } + +#[test] +#[cfg(not(target_vendor = "apple"))] +fn test_from_option() { + use std::os::unix::fs::MetadataExt; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let groups = nix::unistd::getgroups().unwrap(); + // Skip test if we don't have at least two different groups to work with + if groups.len() < 2 { + return; + } + let (first_group, second_group) = (groups[0], groups[1]); + + at.touch("test_file"); + scene + .ucmd() + .arg(first_group.to_string()) + .arg("test_file") + .succeeds(); + + // Test successful group change with --from + scene + .ucmd() + .arg("--from") + .arg(first_group.to_string()) + .arg(second_group.to_string()) + .arg("test_file") + .succeeds() + .no_stderr(); + + // Verify the group was changed + let new_gid = at.plus("test_file").metadata().unwrap().gid(); + assert_eq!(new_gid, second_group.as_raw()); + + scene + .ucmd() + .arg("--from") + .arg(first_group.to_string()) + .arg(first_group.to_string()) + .arg("test_file") + .succeeds() + .no_stderr(); + + let unchanged_gid = at.plus("test_file").metadata().unwrap().gid(); + assert_eq!(unchanged_gid, second_group.as_raw()); +} + +#[test] +#[cfg(not(any(target_os = "android", target_os = "macos")))] +fn test_from_with_invalid_group() { + let (at, mut ucmd) = at_and_ucmd!(); + at.touch("test_file"); + #[cfg(not(target_os = "android"))] + let err_msg = "chgrp: invalid user: 'nonexistent_group'\n"; + #[cfg(target_os = "android")] + let err_msg = "chgrp: invalid user: 'staff'\n"; + + ucmd.arg("--from") + .arg("nonexistent_group") + .arg("staff") + .arg("test_file") + .fails() + .stderr_is(err_msg); +} + +#[test] +#[cfg(not(target_vendor = "apple"))] +fn test_verbosity_messages() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let groups = nix::unistd::getgroups().unwrap(); + // Skip test if we don't have at least one group to work with + if groups.is_empty() { + return; + } + + at.touch("ref_file"); + at.touch("target_file"); + + scene + .ucmd() + .arg("-v") + .arg("--reference=ref_file") + .arg("target_file") + .succeeds() + .stderr_contains("group of 'target_file' retained as "); +} + +#[test] +#[cfg(not(target_vendor = "apple"))] +fn test_from_with_reference() { + use std::os::unix::fs::MetadataExt; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let groups = nix::unistd::getgroups().unwrap(); + if groups.len() < 2 { + return; + } + let (first_group, second_group) = (groups[0], groups[1]); + + at.touch("ref_file"); + at.touch("test_file"); + + scene + .ucmd() + .arg(first_group.to_string()) + .arg("test_file") + .succeeds(); + + scene + .ucmd() + .arg(second_group.to_string()) + .arg("ref_file") + .succeeds(); + + // Test --from with --reference + scene + .ucmd() + .arg("--from") + .arg(first_group.to_string()) + .arg("--reference=ref_file") + .arg("test_file") + .succeeds() + .no_stderr(); + + let new_gid = at.plus("test_file").metadata().unwrap().gid(); + let ref_gid = at.plus("ref_file").metadata().unwrap().gid(); + assert_eq!(new_gid, ref_gid); +} From 8e9a4b5f9a4c1d8f6bc75de64eb2e0c676822d9c Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jan 2025 21:13:07 +0100 Subject: [PATCH 3/6] chgrp: adjust the output with group --- src/uucore/src/lib/features/perms.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3ecac511e..9838de109 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -468,7 +468,11 @@ impl ChownExecutor { } _ => entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), }; - println!("ownership of {} retained as {}", path.quote(), ownership); + if self.verbosity.groups_only { + println!("group of {} retained as {}", path.quote(), ownership); + } else { + println!("ownership of {} retained as {}", path.quote(), ownership); + } } } } From d76c561516670eba4240d718b00a57710828f279 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jan 2025 21:17:04 +0100 Subject: [PATCH 4/6] chgrp: support the --from=:X syntax --- src/uu/chgrp/src/chgrp.rs | 33 ++++++++++++++++++-------- tests/by-util/test_chgrp.rs | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index 9fc0cb2ff..f93fff646 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -19,6 +19,24 @@ use std::os::unix::fs::MetadataExt; const ABOUT: &str = help_about!("chgrp.md"); const USAGE: &str = help_usage!("chgrp.md"); +fn parse_gid_from_str(group: &str) -> Result { + if let Some(gid_str) = group.strip_prefix(':') { + // Handle :gid format + gid_str + .parse::() + .map_err(|_| format!("invalid group id: '{}'", gid_str)) + } else { + // Try as group name first + match entries::grp2gid(group) { + Ok(g) => Ok(g), + // If group name lookup fails, try parsing as raw number + Err(_) => group + .parse::() + .map_err(|_| format!("invalid group: '{}'", group)), + } + } +} + fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { let mut raw_group: String = String::new(); let dest_gid = if let Some(file) = matches.get_one::(options::REFERENCE) { @@ -38,26 +56,21 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { if group.is_empty() { None } else { - match entries::grp2gid(group) { + match parse_gid_from_str(group) { Ok(g) => Some(g), - _ => { - return Err(USimpleError::new( - 1, - format!("invalid group: {}", group.quote()), - )) - } + Err(e) => return Err(USimpleError::new(1, e)), } } }; // Handle --from option let filter = if let Some(from_group) = matches.get_one::(options::FROM) { - match entries::grp2gid(from_group) { + match parse_gid_from_str(from_group) { Ok(g) => IfFrom::Group(g), - _ => { + Err(_) => { return Err(USimpleError::new( 1, - format!("invalid group: {}", from_group.quote()), + format!("invalid user: '{}'", from_group), )) } } diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 086f21cb0..c39824907 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -547,3 +547,49 @@ fn test_from_with_reference() { let ref_gid = at.plus("ref_file").metadata().unwrap().gid(); assert_eq!(new_gid, ref_gid); } + +#[test] +#[cfg(not(target_vendor = "apple"))] +fn test_numeric_group_formats() { + use std::os::unix::fs::MetadataExt; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let groups = nix::unistd::getgroups().unwrap(); + if groups.len() < 2 { + return; + } + let (first_group, second_group) = (groups[0], groups[1]); + + at.touch("test_file"); + + scene + .ucmd() + .arg(first_group.to_string()) + .arg("test_file") + .succeeds(); + + // Test :gid format in --from + scene + .ucmd() + .arg(format!("--from=:{}", first_group.as_raw())) + .arg(second_group.to_string()) + .arg("test_file") + .succeeds() + .no_stderr(); + + let new_gid = at.plus("test_file").metadata().unwrap().gid(); + assert_eq!(new_gid, second_group.as_raw()); + + // Test :gid format in target group + scene + .ucmd() + .arg(format!("--from={}", second_group.as_raw())) + .arg(format!(":{}", first_group.as_raw())) + .arg("test_file") + .succeeds() + .no_stderr(); + + let final_gid = at.plus("test_file").metadata().unwrap().gid(); + assert_eq!(final_gid, first_group.as_raw()); +} From 9c42b8efdc12c2646250f12584e12c1445ec8514 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 12 Jan 2025 22:38:04 +0100 Subject: [PATCH 5/6] chgrp: rename a test for something a bit more explicit --- tests/by-util/test_chgrp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index c39824907..cd40d80be 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -56,7 +56,7 @@ fn test_invalid_group() { } #[test] -fn test_1() { +fn test_error_1() { if getegid() != 0 { new_ucmd!().arg("bin").arg(DIR).fails().stderr_contains( // linux fails with "Operation not permitted (os error 1)" From 4c3e9c893ac45e524770bc4f44e167fbe4ae8a5d Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 19 Jan 2025 23:22:55 +0100 Subject: [PATCH 6/6] chgrp: split the functions into two --- src/uu/chgrp/src/chgrp.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index f93fff646..07d34071c 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -37,8 +37,8 @@ fn parse_gid_from_str(group: &str) -> Result { } } -fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { - let mut raw_group: String = String::new(); +fn get_dest_gid(matches: &ArgMatches) -> UResult<(Option, String)> { + let mut raw_group = String::new(); let dest_gid = if let Some(file) = matches.get_one::(options::REFERENCE) { fs::metadata(file) .map(|meta| { @@ -62,6 +62,11 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { } } }; + Ok((dest_gid, raw_group)) +} + +fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { + let (dest_gid, raw_group) = get_dest_gid(matches)?; // Handle --from option let filter = if let Some(from_group) = matches.get_one::(options::FROM) {