Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chgrp: add option --from #7129

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chgrp: add option --from #7129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f8ea3a
print_verbose_ownership_retained_as: siplify the function
sylvestre Jan 12, 2025
2db0204
chgrp: add option --from
sylvestre Jan 12, 2025
e803a94
chgrp: adjust the output with group
sylvestre Jan 12, 2025
84231b4
chgrp: support the --from=:X syntax
sylvestre Jan 12, 2025
89ff2c5
chgrp: rename a test for something a bit more explicit
sylvestre Jan 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/uu/chgrp/src/chgrp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<u32, String> {
if let Some(gid_str) = group.strip_prefix(':') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the option is --from=:foo, could foo be a group name? This code assumes it is always a numeric group ID, I think. The docs (for chown, but I think it applies to chgrp also) seem to imply that it can be a group name: https://www.gnu.org/software/coreutils/manual/html_node/chown-invocation.html

Also, the docs for chgrp say

Change a file’s ownership only if it has current attributes specified by old-owner. old-owner has the same form as new-owner described above.

new-owner is described in the chown documentation as allowing anything of the form [USER] [: [GROUP]]. If that's true, then we should check for user names and IDs too.

All of these can be fixed separately (and common code refactored from chgrp, chown, and chroot), I just wanted to share my understanding of the GNU documentation.

// Handle :gid format
gid_str
.parse::<u32>()
.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::<u32>()
.map_err(|_| format!("invalid group: '{}'", group)),
}
}
}

fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<GidUidOwnerFilter> {
let mut raw_group: String = String::new();
let dest_gid = if let Some(file) = matches.get_one::<String>(options::REFERENCE) {
Expand All @@ -38,22 +56,28 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<GidUidOwnerFilter> {
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::<String>(options::FROM) {
match parse_gid_from_str(from_group) {
Ok(g) => IfFrom::Group(g),
Err(e) => return Err(USimpleError::new(1, e)),
}
} else {
IfFrom::All
};

Ok(GidUidOwnerFilter {
dest_gid,
dest_uid: None,
raw_owner: raw_group,
filter: IfFrom::All,
filter,
})
}

Expand Down Expand Up @@ -120,6 +144,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')
Expand Down
34 changes: 13 additions & 21 deletions src/uucore/src/lib/features/perms.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,29 +457,21 @@ impl ChownExecutor {

fn print_verbose_ownership_retained_as(&self, path: &Path, uid: u32, gid: Option<u32>) {
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()),
);
}
(_, _, _) => {
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())
}
_ => entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()),
};
if self.verbosity.groups_only {
println!("group of {} retained as {}", path.quote(), ownership);
} else {
println!("ownership of {} retained as {}", path.quote(), ownership);
}
}
}
Expand Down
179 changes: 178 additions & 1 deletion tests/by-util/test_chgrp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand Down Expand Up @@ -417,3 +417,180 @@ 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(target_vendor = "apple"))]
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 group: 'nonexistent_group'\n";
#[cfg(target_os = "android")]
let err_msg = "chgrp: invalid group: 'staff'\n";

ucmd.arg("--from")
.arg("nonexistent_group")
.arg("staff")
.arg("test_file")
.fails()
.stderr_is(err_msg);
}
Comment on lines +468 to +484
Copy link
Contributor

@cakebaker cakebaker Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails on my machine. And I don't know if the error message is deliberately different from GNU chgrp. With GNU it is about an invalid user and not about an invalid group.

$ cargo run --features=unix -q chgrp --from nonexistent_group staff README.md
chgrp: invalid group: 'staff'
$ chgrp --from nonexistent_group staff README.md
chgrp: invalid user: ‘nonexistent_group’


#[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("test_file");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file is not needed as it isn't used anywhere else in this test.

Suggested change
at.touch("test_file");

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);
}

#[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());
}
Loading